Redecleration of NOMINMAX macro on WIN32

[markaren]

algorithm.hpp algorithm.cpp declares NOMINMAX without checking if it already is declared. This causes issues with 3rd party headers such as boost and thrift.

Either a guard should be added or we just move it to the CMake script (cleanest solution IMO) as algorithm.hpp isn't necessary the only header that needs that macro.

[kyllingstad]

Are you sure this is the problem? I've searched the code in master now, and the only NOMINMAX definition I can find is in algorithm.cpp, where it should have no effect at all on client code.

IMO, the preferred convention is to define NOMINMAX in .cpp files, not in headers or in CMake, and then only in .cpp files that fulfil the following two conditions:

• Windows.h is included (directly or indirectly)
• The min or max symbols are used

The drawback of defining it as a build-global setting with CMake is that it could easily leak out as a hidden requirement of the public API. (This is still a possibility with the convention suggested above, but it's smaller and more contained.)

[markaren]

Are you sure this is the problem? I've searched the code in master now, and the only NOMINMAX definition I can find is in algorithm.cpp, where it should have no effect at all on client code.

Sorry, you are right. It was defined in the .cpp file. But the problem of re-declaration is still present. If you think putting it in the CMake script is a bad idea, we should at least put a guard on it.

[kyllingstad]

The NOMINMAX definition is placed at the top of algorithm.cpp, before any #include directives or anything else, so it will be the very first thing the compiler (more precisely, the preprocessor) sees when it compiles that file. Therefore, NOMINMAX cannot be already defined at that point, unless it has been defined from the outside (e.g. in CMake). We aren't doing that, as far as I know, so any errors you're seeing must come from somewhere else.

[markaren]

And? I'm experiencing this error, and it can be resolved by a simple guard. Why should it not be added?

Note that this is not an issue with the current master + conan. But it can manifest itself with new 3rd party headers. It also appears when you add BOOST_FIBERS_NO_ATOMICS, but that macro is bugged anyway.

[kyllingstad]

I am just trying to understand what is going on, mainly to rule out some deeper problem (e.g. with our build system). I really don't understand how adding a guard will fix any issue, and here's why:

The definition of NOMINMAX in algorithm.cpp is literally the first thing the compiler sees. Nothing is defined at that point beyond what is defined by the compiler itself or the build system. No Boost code or any third-party headers have been processed. In other words, even if you replace the existing code with

#if defined(_WIN32) and !defined(NOMINMAX)
#define NOMINMAX
#endif

then that should be exactly equivalent to what is there now, because defined(NOMINMAX) should always be false.

If you are getting a compilation error, and that error is fixed by adding the extra guard, then there may be something fishy going on with our build system, and I'd like to see if we can fix that rather than add what should otherwise be a no-op to the source code.

[markaren]

It sounded like this was a non-issue.

Btw

#if defined(_WIN32) and !defined(NOMINMAX)
#define NOMINMAX
#endif 

seems to be invalid code.

#ifdef _WIN32
#ifndef NOMINMAX
#    define NOMINMAX
#endif
#endif

works.

[kyllingstad]

Sorry, what I meant was:

#if defined(_WIN32) && !defined(NOMINMAX)