Page MenuHomePhabricator

[libc++] Consistently unparenthesize `numeric_limits<T>::max`. NFCI.
ClosedPublic

Authored by Quuxplusone on Nov 27 2020, 11:20 AM.

Details

Summary

I think people were sometimes parenthesizing (foo::max)() out of misplaced concern that an unparenthesized foo::max() would trip up Windows' max(a,b) macro. However, this is not the case: max(a,b) should be tripped up only by an unparenthesized call to foo::max(a,b), and in fact we already do _VSTD::max(a,b) all over the place anyway without any guards.

However, in order to do it without guards, we must also wrap the header in _LIBCPP_PUSH_MACROS, which <span> was not.

(For reference: The charconv pieces were originally added in D41458.)

Diff Detail

Event Timeline

Quuxplusone created this revision.Nov 27 2020, 11:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 27 2020, 11:20 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone requested review of this revision.Nov 27 2020, 11:20 AM
ldionne accepted this revision.Nov 27 2020, 11:20 AM

Don't wait for CI, this is fine. Thanks!

This revision is now accepted and ready to land.Nov 27 2020, 11:20 AM
Quuxplusone planned changes to this revision.EditedNov 27 2020, 12:05 PM

Actually, this does break libcxx/test/libcxx/min_max_macros.compile.pass.cpp — but now I'm trying to figure out how the existing use of min() and max(), e.g. in <istream>, doesn't break that test!

EDIT: oh, that's easy: that test skips <istream> and <latch> and so on. But <chrono> and <limits> and <random> are all there... how do those headers not break it...?

Quuxplusone edited the summary of this revision. (Show Details)

Ah, I see it now.

$ git grep -L _LIBCPP_PUSH_MACROS ../libcxx/include/ | xargs git grep -l '::max('
../libcxx/include/barrier
../libcxx/include/latch
../libcxx/include/semaphore
../libcxx/include/span

I wouldn't mind making the patch for barrier/latch/semaphore (basically D68480 should have added them to lists-of-all-the-headers in a couple more places than it did, and min_max_macros.compile.pass.cpp was one of those places) — but I suspect you might want to do that yourself, so I won't do it unless you ask me to.

This revision is now accepted and ready to land.Nov 27 2020, 12:22 PM

I wouldn't mind making the patch for barrier/latch/semaphore (basically D68480 should have added them to lists-of-all-the-headers in a couple more places than it did, and min_max_macros.compile.pass.cpp was one of those places)

@ldionne, should that be added to NOTES.txt/(future)contributing.rst?
@Quuxplusone, are there any other places you found?

I wouldn't mind making the patch for barrier/latch/semaphore (basically D68480 should have added them to lists-of-all-the-headers in a couple more places than it did, and min_max_macros.compile.pass.cpp was one of those places)

@ldionne, should that be added to NOTES.txt/(future)contributing.rst?
@Quuxplusone, are there any other places you found?

Technically no, I hadn't looked. But now that I've looked, yes, of the places I know as "big lists of headers", the new synchronization-primitive headers are missing from exactly two of them:
include/CMakeLists.txt (OK)
include/module.modulemap (OK)
test/libcxx/double_include.sh.cpp (OK)
test/libcxx/min_max_macros.compile.pass.cpp (missing)
test/libcxx/no_assert_include.compile.pass.cpp (missing)

...

[...] — but I suspect you might want to do that yourself, so I won't do it unless you ask me to.

Feel free to go ahead! Thank you both @Quuxplusone and @curdeius for straightening this out. Libc++ doesn't get a lot of this kind of love, and it needs it!