Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

min/max compilation issues with MSVC #68

Open
stephenberry opened this issue Sep 16, 2024 · 3 comments
Open

min/max compilation issues with MSVC #68

stephenberry opened this issue Sep 16, 2024 · 3 comments

Comments

@stephenberry
Copy link

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.

@Alcaro
Copy link

Alcaro commented Sep 16, 2024

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.

@stephenberry
Copy link
Author

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.

@jk-jeon
Copy link
Owner

jk-jeon commented Sep 16, 2024

@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants