boostorg / histogram

Fast multi-dimensional generalized histogram with convenient interface for C++14
Boost Software License 1.0
313 stars 74 forks source link

add parentheses around min and max #377

Closed jhcarl0814 closed 1 year ago

jhcarl0814 commented 1 year ago

add parentheses around min and max to suppress macro invocation https://stackoverflow.com/questions/6884093/warning-c4003-not-enough-actual-parameters-for-macro-max-visual-studio-2010 https://stackoverflow.com/questions/7449620/how-to-call-stdmin-when-min-has-been-defined-as-a-macro https://stackoverflow.com/questions/22744262/cant-call-stdmax-because-minwindef-h-defines-max https://stackoverflow.com/questions/11544073/how-do-i-deal-with-the-max-macro-in-windows-h-colliding-with-max-in-std

HDembinski commented 1 year ago

Thanks for this. Did you just spot these or did you use an algorithm to find all cases of unprotected min/max?

HDembinski commented 1 year ago

Thanks, these look all good. It is rediculous that Windows enforces this workaround on everyone.

I don't like sprinkling the code with parenthesis because the reason is not clear unless you know about the Windows bug, but the only more obvious fix is incredibly verbose, so it is better this way. It is the technique used by Boost.Config. In max(...), one insert BOOST_PREVENT_MACRO_SUBSTITUATION between max and (...), like so:

        ct width = max BOOST_PREVENT_MACRO_SUBSTITUTION (ct(1),
jhcarl0814 commented 1 year ago

@HDembinski Thank you for pointing out the standard way of doing the workaround.

I was copying the test code into the beginning of main of an existing project (wanted to use IDE debugger) when fixing the bugs of the PR. There were some other test code for other stuffs that caused me to #include<windows.h> in the past, that's how I got the min and max errors.

So I global searched min( and max( and added the parentheses. (Oh no! I missed the cases where there might be white spaces and comments between the identifier and parenthesis!) (I agree that the windows.h's macro is really annoying.)