This patch improves how libc++ handles min/max macros within the headers. Previously libc++ would undef them and emit a warning.
This patch changes libc++ to use #pragma push_macro to save the macro before undefining it, and #pragma pop_macro to restore the macros and the end of the header.
Details
Details
- Reviewers
mclow.lists bcraig compnerd EricWF - Commits
- rGa016efb1dcda: [Libc++] Use #pragma push_macro/pop_macro to better handle min/max on Windows
rCXX304357: [Libc++] Use #pragma push_macro/pop_macro to better handle min/max on Windows
rL304357: [Libc++] Use #pragma push_macro/pop_macro to better handle min/max on Windows
Diff Detail
Diff Detail
Event Timeline
Comment Actions
- Remove failing test. This patch cannot be easily tested because the system's libc might also mess with the definitions of min and max.
| include/__threading_support | ||
|---|---|---|
| 630 | Missing _LIBCPP_POP_MACROS | |
Comment Actions
I like the warning that you generate for min and max macros existing. Is the push_macro / pop_macro the right way to go though? You could throw extra parenthesis around the declaration and usages of min/max to avoid macro expansion.
#define add(x,y) (x)-(y)
template <typename T>
T (add)(T x, T y) // parens around add to suppress macro
{
return x+y;
}
int main() {
return
(add)(5, 4) + // uses function
add(5, 4); // uses macro
}Comment Actions
Only on warn on platforms where we don't have #pragma push_macro/pop_macro.
Is the push_macro / pop_macro the right way to go though? You could throw extra parenthesis around the declaration and usages of min/max to avoid macro expansion.
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.
Also there are a lot of call sites and declarations for min and max. I think this approach is less invasive and more consistent. We're obviously not going to
use (foo)(...) syntax everywhere, so lets use it nowhere.
Comment Actions
- Fix missing _LIBCPP_POP_MACROS in <__threading_support>.
- Add test that each header is unaffected by min and max macros and that the macros themselves are unaffected.
Comment Actions
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.
| include/shared_mutex | ||
|---|---|---|
| 128–136 | Shouldn't the push macro be somewhere below this potential include? | |
Comment Actions
I think that we should sink the min/max checks into __undef_macros. I don't like the idea of littering that check everywhere.
| include/__config | ||
|---|---|---|
| 1218 | clang-format these please? It does a nice job of aligning the \. | |
Comment Actions
I would much rather litter at the cost of the implementation than needlessly include __undef_macros in every header; Especially because
it's almost always unneeded. I realize it's ugly internally but I think it's worth it. I'll attempt to write some compile time tests to ensure
I'm not making a mountain out of a molehill but could you also re-consider @compnerd?
Comment Actions
- Remove the external include guards for __undef_macros as requested by @compnerd.
I couldn't come up with a easy test case that showed any notable difference having the external guards.
clang-format these please? It does a nice job of aligning the \.