dragonbox icon indicating copy to clipboard operation
dragonbox copied to clipboard

min/max compilation issues with MSVC

Open stephenberry opened this issue 1 year ago • 3 comments

MSVC windows headers define macros for min and max, which conflict with any use of min and max names in code, like dragonbox. An elegant solution is to add parenthesis, for example change std::max(a, b) to (std::max)(a, b), this prevents macro expansion and doesn't change the logic of the code.

stephenberry avatar Sep 16 '24 13:09 stephenberry

The easiest fix is tell people to #define NOMINMAX before including windows.h, or include Dragonbox before windows.h. But that's kinda impolite, so I agree with doing something.

(std::min)(a,b) works, yes. Another option is using std::min;, then call min(a,b) and don't worry too much about whether min is a macro or not. A third option is #pragma push_macro, like https://github.com/microsoft/STL/blob/73b5791e5c9eff1ece3ce593571fb30c31bf08d9/stl/inc/yvals_core.h#L695.

Personally I'd go for push_macro, to keep the msvc ugliness localized and not spread throughout the codebase, but your choice.

Alcaro avatar Sep 16 '24 13:09 Alcaro

I wouldn't tell users to #define NOMINMAX, because it's much better to just solve the problem and users will inevitably miss the documentation. But, the other suggestions @Alcaro proposed are also good.

stephenberry avatar Sep 16 '24 14:09 stephenberry

@stephenberry @Alcaro Thanks for the suggestions.

My initial thought when I first wrote it was that "well, any sane developer should do #define NOMINMAX before including <Windows.h> of course" but that was because I didn't know that this issue is workaround-able with such a simple parenthesis trick.

using std::min doesn't work because dragonbox.h doesn't include algorithm header and it won't. (It's one of the heaviest headers in the entire standard library thanks to the addition of std::ranges , so I want to avoid it if possible in this kinds of low-level header libraries.)

The push_macro trick indeed looks very elegant and I appreciate you for bringing it up (I didn't know about it), but I don't want to rely on MSVC-specific language extensions because I believe MSVC is not the only compiler which compiles the contents in Windows.h.

After a search, it seems actually GCC also supports it for compatibility with MSVC (and probably Clang as well), but I would just get my hands dirty with the parenthesis trick rather than relying on a language extension without a fallback.

PR is very much welcome, or I'll do it myself later if I have time.

Thanks again!

jk-jeon avatar Sep 16 '24 17:09 jk-jeon

Fixed in 39902e6244f459b3f4851ae6763dfa09df6faf3b. Please let me know if you find this issue persists, thanks!

jk-jeon avatar Oct 28 '24 20:10 jk-jeon