Page MenuHomePhabricator

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

Authored by EricWF on May 10 2017, 4:31 PM.

Details

Summary

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.

Diff Detail

Repository
rL LLVM

Event Timeline

EricWF created this revision.May 10 2017, 4:31 PM
EricWF updated this revision to Diff 98561.May 10 2017, 4:34 PM
  • Remove failing test. This patch cannot be easily tested because the system's libc might also mess with the definitions of min and max.
EricWF updated this revision to Diff 98562.May 10 2017, 4:47 PM
  • Fix unterminated preprocessor directive.
EricWF added inline comments.May 11 2017, 2:12 AM
include/__threading_support
632 ↗(On Diff #98562)

Missing _LIBCPP_POP_MACROS

bcraig edited edge metadata.May 11 2017, 6:36 AM

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
}

I like the warning that you generate for min and max macros existing.

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.

EricWF updated this revision to Diff 98725.May 11 2017, 11:35 PM
  • 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.

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 ↗(On Diff #98725)

Shouldn't the push macro be somewhere below this potential include?

compnerd requested changes to this revision.May 14 2017, 12:38 PM

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 ↗(On Diff #98725)

clang-format these please? It does a nice job of aligning the \.

This revision now requires changes to proceed.May 14 2017, 12:38 PM

I think that we should sink the min/max checks into __undef_macros. I don't like the idea of littering that check everywhere.

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?

EricWF updated this revision to Diff 100269.May 25 2017, 10:51 AM
EricWF edited edge metadata.
  • 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.

EricWF updated this revision to Diff 100361.May 25 2017, 8:48 PM
  • Fix whitespace in __config.

@compnerd Any final thoughts?

EricWF accepted this revision.May 31 2017, 2:51 PM

LGTM.

This revision was automatically updated to reflect the committed changes.