search

max0x7ba / atomic_queue

C++ lockless queue.
MIT License
1.51k stars 180 forks source link

workaround for "min max" (when including Windows.h) #37

Closed ptahmose closed 2 years ago

ptahmose commented 2 years ago

Putting brackets around "std::max" prevents problems when including Windows.h (which #define's this) - c.f. https://stackoverflow.com/questions/5004858/why-is-stdmin-failing-when-windows-h-is-included.

max0x7ba commented 2 years ago

Thank you for your contribution.

Shouldn't you just define Windows macro NOMINMAX prior to including that notorious standards breaking windows header file to avoid having to patch all and any standard complying 3rd-party header files?

ptahmose commented 2 years ago

Yeah, sure, NOMINMAXsolves the issue at the other end, but of course at the risk of breaking other stuff.

You must understand that changing a 3rd-party library because of an unspecified imaginary problem is rather poor justification.

In my experience, this "bracket-trick" is without side-effects, and therefore makes this project more versatile and more easy to use. So I'd argue it is a valuable addition for atomic_queue. But sure, I can understand that the additional clutter is undesirable and can be seen as covering up an evident flaw (in Windows.h).

This is unnecessary clutter for me. I do not develop on Windows and have no toolchain nor continuous integration for maintaining it.

For my case, I came to conclusion that patching atomic_queue is the most prudent way of resolving the issue, so I'd of course be delighted if you'd accept this PR. But as said, I can fully understand if you decide against it.

I appreciate your contribution but changing any 3rd-party library header files because of problems created by including Windows.h while unwilling to define macro NOMINMAX for no good reason, is neither prudent nor reasonable, your logic doesn't make sense to me.

Following your logic you will have to patch all your 3rd-party libraries broken by Windows macros, which is rather quixotic. That's why NOMINMAX macro simple solution exists to solve your problem of your own making with one line of code. Windows.h is a notoriously toxic header file and adding clutter to other libraries makes the problem more expensive for everyone. The problem must rather be solved at the root of it, there is no cheaper or more prudent solution.

I am sorry, I cannot accept this patch.