AcademySoftwareFoundation / openvdb

OpenVDB - Sparse volume data structure and tools
http://www.openvdb.org/
Mozilla Public License 2.0
2.62k stars 647 forks source link

Fix OpenVDB Python module with NumPy compilation on Windows MSVC #1755

Open MichaelMoroz opened 8 months ago

MichaelMoroz commented 8 months ago

I couldn't compile the OpenVDB python module with NumPy due to ssize_t missing on MSVC. This hopefully fixes it.

linux-foundation-easycla[bot] commented 8 months ago

CLA Signed

The committers listed above are authorized under a signed CLA.

jmlait commented 7 months ago

Thank you for this. Unfortunately we need two TLAs: CLA and DCO. Can you please re-do the commit with --signoff so it passes the DCO test? Thank you.

NeedsMoar commented 7 months ago

Just a comment but ssize_t is absent on Windows because it's a POSIX specific data type which is poorly defined in the POSIX standard (-1 is the only guaranteed negative value, there isn't a guarantee about the size of the data type). The Windows header you used defines it as LONG_PTR which is int64_t on 64 bit windows and int32_t otherwise, but the leftover POSIX functions on Windows return int where ssize_t would have been used.

AFAIK it's not defined or used in the newest or any previous version of the C standard so the proper fix is to call the Windows version of whatever POSIX function is involved in returning it. Python does a fairly good job of that internally even where it uses a posix name in code... If numpy is returning ssize_t somewhere on Windows, it's an error, and it would be preferable to file a bug with them to conditionalize that or just use standard C types (even on Linux really, it's not like they're paying to be POSIX certified on a yearly basis or much else is going on in the UNIX world but BSDs) rather than propagating what amounts to an error somewhere, IMO.

francoiscoulon commented 6 months ago

Hi,

I had the same issue and I have food for thoughts here with my humble suggestion down at the end that I used to fix this on my end.

  1. Here the preprocessor macro is using _MSC_VER limiting to visual studio compiler. To work with _WIN32, we'd have to look for another solution, leading to:
  2. Understand what ssize_t really means in both posix and windows and if there is a standard solution.
    • ssize_t by posix: The type ssize_t shall be capable of storing values at least in the range [-1, {SSIZE_MAX}] and shall be signed integer type.
      • SSIZE_T in msvc BaseTsd.h: A signed version of SIZE_T. This type is declared in BaseTsd.h as follows: typedef LONG_PTR SSIZE_T;
      • Standard C++ library has std::ptrdiff_t: [It] acts as the signed counterpart of size_t.

With all these information(links included) we see we should be able to use std::ptrdiff_t.

Quick workaround more "generic" could be:

#if defined (_WIN32)
#include <cstddef> // for std::ptrdiff_t
#define ssize_t std::ptrdiff_t
#endif

A fix could be to use std::ptrdiff_t instead of ssize_t all over.

I'm happy to create a MR with either these suggestions if you think they're improving Michael's suggestion...