[PATCH] D33080: [Libc++] Use #pragma push_macro/pop_macro to better handle min/max on Windows

[Ben Craig via Phabricator via cfe-commits]

bcraig added a comment.

> I noticed that the Windows STL headers have to do this dance with `new` (even though they do `(foo)(...)` for `min` and `max`). If we're going to need
>  to guard against a bunch of macros I would like to use a single approach.  Other than updating the `#if defined(min) || defined(max)` lines it's trivial to guard
>  against additional macros.

I think the guarding of 'new' is done because of user code, not because of headers #defining new.  There is a lot of old Windows code out there that would define 'new' to something else in order to get extra instrumentation.  I want to say this was even done in some of the MFC example 'project templates'

One of the things I don't like about push / pop macro is that it isn't the same as doing nothing with the macro.  If something between the push and pop specifically wanted to use or modify the macro, the pop macro will destroy that state.  Granted, in this situation, that seems both unlikely, and it would be evil heaped upon evil.

With push and pop macro, you also need to be very careful about how things are positioned.  The push and undef need to happen after all the other includes in the header are processed, and care needs to be taken not to include anything offensive within the bounds of the push and pop.  You also need to remember to do the pop.

push and pop macro are acceptable.  I still have a preference for the parenthesis approach though.



================
Comment at: include/shared_mutex:128-136
+_LIBCPP_PUSH_MACROS
+#if defined(min) || defined(max)
+# include <__undef_macros>
+#endif
+
+
 #if _LIBCPP_STD_VER > 11 || defined(_LIBCPP_BUILDING_SHARED_MUTEX)
----------------
Shouldn't the push macro be somewhere below this potential include?


https://reviews.llvm.org/D33080