CesiumGS / cesium-native

Apache License 2.0
414 stars 210 forks source link

[MODIFY] Avoid conflicts with the std library max function #930

Closed No-needto-recall closed 1 month ago

No-needto-recall commented 1 month ago

Title: Correctly Invoke std::numeric_limits::max as a Function to Prevent Macro Conflicts

Body:

Problem Description The existing template implementation in Cesium-native uses std::numeric_limits::max without parentheses. This can lead to conflicts with preprocessor macros, especially in environments where max is defined as a macro, resulting in incorrect preprocessor expansion and compilation errors.

Proposed Change To prevent this issue and ensure that max is treated as a function returning the maximum value of type T, I propose modifying the syntax to explicitly include function-call parentheses. This change will avoid macro conflicts and ensure consistent behavior across various compilers.

Implementation Details Here is the specific change applied to the template that handles AccessorView<AccessorTypes::VEC2>:

Before

u /= std::numeric_limits<T>::max;  // Potential macro conflict
v /= std::numeric_limits<T>::max;  // Potential macro conflict

After

u /= (std::numeric_limits<T>::max)();  // Clear function call with parentheses
v /= (std::numeric_limits<T>::max)();  // Clear function call with parentheses

Expected Impact This minor syntax correction will eliminate the risk of macro interference by ensuring that std::numeric_limits::max is treated strictly as a function. This is particularly important for maintaining the stability and reliability of the template code across different compilation environments that may define max as a macro.

Request for Feedback I invite feedback from the community on this change, particularly from those who may have experienced similar macro conflicts or have suggestions on ensuring template code robustness.

Conclusion Thank you for considering this improvement. I look forward to any feedback and hope this modification will enhance the clarity and robustness of the Cesium-native project's code.

kring commented 1 month ago

Thanks for the PR @No-needto-recall. I don't think we will accept this PR, though. As you can see in your diff, there were already function call parenthesis in the original code. You've just added some additional, unnecessary ones.

If your own code is defining a macro named max, I suggest you change this. It's very poor practice. If some other third-party header is doing this (a common culprit is windows.h), the best practice is to isolate the code that needs that problematic header into its own .cpp file and make sure that .cpp doesn't include anything from cesium-native. Think of it as definining an interface to that other misbehaving code that protects the rest of your code base from it.

If you're specifically fighting with windows.h, adding #define NOMINMAX before the #include <windows.h> will solve it.

No-needto-recall commented 1 month ago

Thanks for the PR @No-needto-recall. I don't think we will accept this PR, though. As you can see in your diff, there were already function call parenthesis in the original code. You've just added some additional, unnecessary ones.

If your own code is defining a macro named max, I suggest you change this. It's very poor practice. If some other third-party header is doing this (a common culprit is windows.h), the best practice is to isolate the code that needs that problematic header into its own .cpp file and make sure that .cpp doesn't include anything from cesium-native. Think of it as definining an interface to that other misbehaving code that protects the rest of your code base from it.

If you're specifically fighting with windows.h, adding #define NOMINMAX before the #include <windows.h> will solve it.

Thank you for your reply and suggestions!

Regarding the addition of parentheses, my original intention was to ensure that std::numeric_limits::max could be explicitly called as a function in compilation environments where macro definitions might interfere. I have rechecked the original code and compilation environment, and indeed found that in my specific scenario (which might include certain third-party libraries or project settings), the original code did not correctly prevent macro conflicts.

However, based on your suggestion, using #define NOMINMAX or isolating the related code is indeed a clearer and more professional solution. I will adjust my project settings accordingly and will withdraw this PR.

Thank you very much for your guidance and assistance! I look forward to more interactions and cooperation in the future.