build: Add NOMINMAX definition for MSVC compatibility

[siposcsaba89]

• Build system improvement:
  • Added #pragma push_macro/#pragma pop_macro to prevent conflicts with the min and max macros defined in Windows headers.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.60%. Comparing base (2a15fa0) to head (472f357).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #478      +/-   ##
==========================================
- Coverage   96.64%   96.60%   -0.04%     
==========================================
  Files          33       33              
  Lines        2622     2622              
==========================================
- Hits         2534     2533       -1     
- Misses         88       89       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[stotko]

Thanks! I'm a bit hesitant to merge this fix since I don't believe that the proposed preprocessor definition should belong to stdgpu. In fact, the culprit is the <windows.h> header which defines non-standard macros for min and max that would clash with the stdgpu's algorithm.h and limits.h headers. However, <windows.h> is not directly used by stdgpu and unconditionally defining NOMINMAX may break cases where this case is already accounted for. Could you provide more details on your use case?

[siposcsaba89]

Thanks! I'm a bit hesitant to merge this fix since I don't believe that the proposed preprocessor definition should belong to stdgpu. In fact, the culprit is the <windows.h> header which defines non-standard macros for min and max that would clash with the stdgpu's algorithm.h and limits.h headers. However, <windows.h> is not directly used by stdgpu and unconditionally defining NOMINMAX may break cases where this case is already accounted for. Could you provide more details on your use case?

Sorry, thad I did not give you more detailed description. I am trying to port our project to compile with cuda 13, and I belive that thrust and cuda in version 13 includes windows.h or some header from windows that defines min, max, which breaks our build. I understand, that you are hesitant, and I tought about that too, that it could break other cases, could give warning from already defined macros, etc., but if I do not set it, it won't compile. Maybe a pragma pop, push solution would be better, that's how nvidia does it, so I think you are right not to merge it, let me see if I can provide a better solution.

[siposcsaba89]

I just made a #pragma push_macro/#pragma pop_macro variation, needed to add to two files. I am not sure, that it is better or not, it solves the problem locally insted of global define, but maybe it would be better to put these in a header file. I am not sure. But for now I think it is the best I can come up with.

[stotko]

Thanks for coming up with a local version. Limiting the scope to only the necessary areas is the right direction 👍 Since this is a Windows-only issue, I suggest to further limit the scope to that particular platform even though it should have no effect on other platforms.

[siposcsaba89]

Thanks for coming up with a local version. Limiting the scope to only the necessary areas is the right direction 👍 Since this is a Windows-only issue, I suggest to further limit the scope to that particular platform even though it should have no effect on other platforms.

With test to compile I also needed to add it to several files, so decided to create a header push/pop pair, but it counts where the user includes them, but clang-format keep fighting the order. I am not sure what is the best solution here.

What do you think about a private NOMINMAX definition to be able to compile stdgpu (for example vcpkg, as we do it), then everyone should handle NOMINMAX in their own project? (Maybe it would be cleaner than putting push/pop everywhere)

[stotko]

With test to compile I also needed to add it to several files, so decided to create a header push/pop pair, but it counts where the user includes them, but clang-format keep fighting the order. I am not sure what is the best solution here.

What do you think about a private NOMINMAX definition to be able to compile stdgpu (for example vcpkg, as we do it), then everyone should handle NOMINMAX in their own project? (Maybe it would be cleaner than putting push/pop everywhere)

Yes, something like the following (for all targets in question) would be better than the header approach:

if(CMAKE_SYSTEM_NAME STREQUAL "Windows")
    target_compile_definitions(stdgpu PRIVATE NOMINMAX)
endif()

After looking into thrust (and libcudacxx), it looks like this location could be responsible for the issues: https://github.com/NVIDIA/cccl/blob/v2.8.0/libcudacxx/include/cuda/std/__thread/threading_support_win32.h#L28
Could you find out which exact CUDA version introduced the compilation issues? The header in question seems to be available with CUDA 12.9 and newer.

[siposcsaba89]

With test to compile I also needed to add it to several files, so decided to create a header push/pop pair, but it counts where the user includes them, but clang-format keep fighting the order. I am not sure what is the best solution here.
What do you think about a private NOMINMAX definition to be able to compile stdgpu (for example vcpkg, as we do it), then everyone should handle NOMINMAX in their own project? (Maybe it would be cleaner than putting push/pop everywhere)

Yes, something like the following (for all targets in question) would be better than the header approach:

if(CMAKE_SYSTEM_NAME STREQUAL "Windows")
    target_compile_definitions(stdgpu PRIVATE NOMINMAX)
endif()

After looking into thrust (and libcudacxx), it looks like this location could be responsible for the issues: https://github.com/NVIDIA/cccl/blob/v2.8.0/libcudacxx/include/cuda/std/__thread/threading_support_win32.h#L28 Could you find out which exact CUDA version introduced the compilation issues? The header in question seems to be available with CUDA 12.9 and newer.

Yes, I updated from cuda 12.6 to 13.1 when I encountered the problem, so I think you are right, that it was CUDA version 12.9 which introduced this issue.

Updated the PR to add NOMINAX as private definision on Windows.