Re-evaluate the use of the `NOMINMAX` coding pattern when including Windows.h across the SDK

[ahsonkhan]

From @LarryOsterman:

We should never define NOMINMAX, instead you should use the (min) trick Anton used.

I understand that this is not one of our headers, but we should get out of the habit of using min and instead use (min) throughout the SDK. For better or worse, our customers absolutely can (and will) include windows.h before they include our SDK headers, which means that the NOMINMAX definition will be irrelevant, since the min and max macros would have already been defined.

An alternative is to include a #undef min at the beginning of source files, possibly as:

#if defined(min)
#undef min
#endif

to make the removal of the macro explicit.

That way we relegate the use of the (min) pattern to our headers only (where we cannot control what our customers have previously included).

[antkmsft]

I would disagree, do define #define NOMINMAX before including windows.h. If users do want the min and max macro (and I can't imagine many people lining up to get these macros defined, especially in context of C++; especially if you're a beginner, the compiler error is not a pleasant experience), they would #include <windows.h> before #include <azure/.... That is why in addition to #define NOMINMAX, in our code, especially in headers, we should write std::min as (std::min), or std::(min) (doesn't matter) - because when our code in being consumed, min and max macros may still be defined despite us defining NOMINMAX. But we should not contribute to that macro being defined. As I said, the user experience is not great, and could be confusing for inexperienced users - you use std::max(), or even do int max = 999;, and you get unrelated errors and you don't know what is going on. Especially if that line has worked before, and now has stopped working after you added Azure SDK to your project. And respectfully, for these reasons, don't #undef min, especially in headers, because that could be the thing that customers wanted to be defined. #define NOMINMAX is not that likable, but you realize it is the best solution once you look into other options.

[LarryOsterman]

There are two cases we care about: 1) Consumption of Windows.H in our headers. This should probably be avoided if possible (the same goes for all platform specific headers), but if inclusion of windows.h is required in our headers, we cannot take any action that would change the preprocessor definitions created by WIndows.h - that means that NOMINMAX should be banned in any azure SDK headers. And regardless of this, we should always use the (foo:min) construct in our headers.

2) Consumption of windows.h in our non-header C++ files: In this case, the use of NOMINMAX isn't harmful, but it is inconsistent with our headers and we should probably be consistent in how we consume the windows headers across the SDK.

[antkmsft]

But if the customer does not have a lot of experience, and has only included #include <azure/keyvault/keys.hpp> for example - now they have min and max macros defined when building on Windows! And if in the customer code there was a line int i = std::min(0, 1); is not the great experience to figure out, why the compiler is complaining (and it's good if it is in their code, and not in some other library that they were using that suddenly started exploding - for them, it will look like"Azure SDK is incopatible with the xxx lib on Windows, I am not committed to any cloud at the moment, so I am going to try an SDK for some other Cloud where I can be successful".

Or, the fix for them would be to search StackOverflow or find an issue in this repo (or maybe we even document this?) and, in the end, to write:

#define NOMINMAX // Even though I am not using any Win32
#include <algorithm>
#include <azure/keyvault/keys.hpp>

int main() {
    int i = std::min(0, 1);
}

If we always #define NOMINMAX before including Windows.h, there is no such problem. We write std::(min) regardless. But including our headers won't pollute customer's environment.

And, should they want to have the macro (I can't imagine there are many people who want it in C++), the fix for them would be

#include <windows.h> // include before the Azure SDK, so that min() and max() macros get defined.

#include <azure/keyvault/keys.hpp>

Otherwise, by default, you don't get these macros.

[LarryOsterman]

My take is that first off, we should never include platform specific headers in Azure SDK headers. The Azure SDK headers should include the minimal set of includes to perform the task and should never directly or indirectly define types that aren't either defined in the Azure SDK or in the C++ standard library.

If we do include platform specific headers, we should have a REALLY good reason for it. And we should work hard to see if there's a way of including those headers in some other form.

If we are using the std::min or std::max macros in our headers, we MUST use the (std::min)/(std::max) formulation in case a developer had previously included windows.h before they included our headers.

Since our headers are always using the parenthetical form, defining NOMINMAX doesn't make a difference - our code is hardened against the Windows min/max definitions.

So we shouldn't define it, since defining the macro doesn't actually help our code.

[antkmsft]

Yes, our code using (std::min)() will never break, but should we do end up including Windows.h in the inc/ headers, customer's code will get broken by including Azure headers. And the error they'll get is not that clear.

I do not think that many C++ developers would want min and max macros defined for them. std::min() and std::max() are superior in every way. Having min and max macros defined does not enable any new scenario for you. There is no Win32 API that you can't invoke if you don't have these macros defined.

Suppose you still want them, which I think is very exotic and niche, you just include Windows.h before Azure headers, or define them yourself. But by default you don't have to do anything and you don't get these macros when including Azure SDK headers.

I totally agree that we should minimize Windows.h inclusion in our inc/ headers, and any 3rd party library as well, whenever possible. Maybe we can get away with forward-declaring some things. It would be good to have an MQ activity to do that.

Then, if we only #define NOMINMAX in the files from src/ directory is not that important - I'd still recommend it, but it would be much more localized/tactical/developer's choice matter.

[ahsonkhan]

FWIW, we only have two instances of this pattern in header files and they are both for WinHTTP (which is reasonable). https://github.com/Azure/azure-sdk-for-cpp/blob/6bc4ee3f0158185eb398ee6ecc5a70ca077d32a6/sdk/core/azure-core/inc/azure/core/http/win_http_transport.hpp#L18-L26

All other instances are in source files. For example: https://github.com/Azure/azure-sdk-for-cpp/blob/6bc4ee3f0158185eb398ee6ecc5a70ca077d32a6/sdk/core/azure-core/src/cryptography/md5.cpp#L7-L17

Based on the outcome of this issue, please update the guidelines to reflect the C++ SDK stance.

[LarryOsterman]

All other instances are in source files.

Unfortunately, that's not quite true. test_proxy_manager.hpp, which is included as a part of test_base.hpp also includes win_http_transport.hpp.

As a consequence, all of our test collateral includes the windows headers on Windows, and it appears that some of the test collateral may have taken dependencies on that.

It's not the public API surface, but it is there throughout the tests. Trying to assess the damage of this.