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

std::min is broken if FMT_HEADER_ONLY is defined. #152

Closed
DevO2012 opened this issue Apr 27, 2015 · 8 comments
Closed

std::min is broken if FMT_HEADER_ONLY is defined. #152

DevO2012 opened this issue Apr 27, 2015 · 8 comments

Comments

@DevO2012
Copy link

Solution is to put flowing lines just before windows include.

#include <windows.h>
#define WIN32_LEAN_AND_MEAN
#define NOMINMAX
@vitaut
Copy link
Contributor

vitaut commented Apr 27, 2015

Thanks for the bug report. I couldn't find any reference to std::min in C++ Format other than in tests which don't use FMT_HEADER_ONLY. Could you post the full error that you get?

@DevO2012
Copy link
Author

The problem is that windows.h will define min and max macros.
It will also include a lot of other garbage that usually nobody need.
This will break any usage of std::min and std::max in later code.
I have added pull request about this.

@DevO2012 DevO2012 reopened this Apr 27, 2015
@newnon
Copy link
Contributor

newnon commented Apr 28, 2015

you can just write int a = (std::min)(10, 20); and min max macros will not work and you don't need NOMINMAX

@vitaut
Copy link
Contributor

vitaut commented Apr 28, 2015

This is a well-known problem with Windows headers and unfortunately I don't see a good way to address this without potentially breaking user code (which can happen if we unconditionally define NOMINMAX) that also includes windows.h. Possible solutions:

  • Use the default non-header-only configuration
  • Define NOMINMAX before including format.h or on a project level

@DevO2012 Will it work for you?

@DevO2012
Copy link
Author

Yes I know that I can write (std::min)(10, 20) but only in my code not in other libraries that use std::min and std::max.

I don't see a good way to address this without potentially breaking user code (which can happen if we unconditionally define NOMINMAX) that also includes windows.h

Can you please elaborate how this can break user code ?
Do you mean that a lost of users still use preprocessor based min and max, this would be big surprise for me ?

Define NOMINMAX before including format.h or on a project level

Ok this seems to be the most safe solution :)

@vitaut
Copy link
Contributor

vitaut commented Apr 29, 2015

Can you please elaborate how this can break user code ?

It can happen if windows.h is included directly or via another header after including format.h and then a min or max macro is used, possibly by code in another header (this can even be C code):

#include <format.h>
#include <windows.h> // can be included in another header, possibly C
max(1, 2); // same as above

Do you mean that a lost of users still use preprocessor based min and max, this would be big surprise for me ?

I hope noone is using min and max macros, but I don't want to break their code if they do. Although they probably deserve their code to be broken =).

Ok this seems to be the most safe solution :)

Cool, I'm closing this issue then, but if you have any ideas on how to improve the situation feel free to reopen it.

@vitaut vitaut closed this as completed Apr 29, 2015
@DevO2012
Copy link
Author

How about this solution ?

# define WIN32_LEAN_AND_MEAN
# define NOMINMAX
# include <windows.h>
# undef  NOMINMAX
# undef  WIN32_LEAN_AND_MEAN

vitaut added a commit that referenced this issue Apr 30, 2015
because they break std::min/max. This only affects non-default
header-only configuration and can be disabled by defining
FMT_WIN_MINMAX.
@vitaut
Copy link
Contributor

vitaut commented Apr 30, 2015

I've implemented something similar in 67ce394. Unfortunately it may still break if someone uses min/max macros because NOMINMAX only applies to the first include of windows.h. But it's probably a lesser evil compared to breaking std::min/max and it can be disabled by defining FMT_WIN_MINMAX. In any case this only applies to the header-only configuration so it's not that important. Thanks @DevO2012.

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