This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Remove workarounds for unsupported GCC and Clang versions
ClosedPublic

Authored by ldionne on Aug 30 2021, 9:32 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Restricted Project
Commits
rGa4cb5aefd545: [libc++] Remove some workarounds for unsupported GCC and Clang versions
Summary

There is probably a lot more we can do, in particular in <type_traits>,
but this removes workarounds that were gated on checking a specific
compiler version.

Diff Detail

Event Timeline

ldionne requested review of this revision.Aug 30 2021, 9:32 AM
ldionne created this revision.
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 30 2021, 9:32 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone added inline comments.
libcxx/include/__config
1249

Does this imply that there are supported versions of GCC where __has_attribute(fallthrough) reports the wrong answer?
Orthogonally, scope creep, I wonder whether fallthrough should be __fallthrough__.

1255–1265

Let's merge these two #ifs.

libcxx/include/new
335

Is this function template missing _LIBCPP_HIDE_FROM_ABI / _LIBCPP_INLINE_VISIBILITY?

libcxx/include/type_traits
3127–3128

Is this implying that there's a supported version of GCC where __has_feature(is_trivially_constructible) returns the wrong answer?

On old line 3344 you completely remove the __has_feature check; is that not appropriate here as well?

3517–3519

I think this diff should be committed by itself as a separate commit, because I don't understand it.
It looks like the code is saying "Trust the compiler's builtin, except on GCC; on GCC the builtin might return true for volatile-qualified types, and we want to make the type trait return false in those cases." But that's the opposite of what you wrote in the comment.

Nit: "Core 2094" should be spelled either "CWG 2094" or "CWG2094" for greppability.
Nit: "volatile qualified" should be "volatile-qualified".

libcxx/src/include/atomic_support.h
27

Surely defined(_LIBCPP_COMPILER_GCC) already implies !defined(__clang__).

libcxx/src/locale.cpp
263–264

FWIW, I recently had cause to try #pragma GCC diagnostic ignored "-Wall" in real code, and I found that it did nothing at all. I suspect -Wextra is similar.
https://stackoverflow.com/questions/32049296/how-to-disable-all-warnings-using-pragma-directives-in-gcc

Try removing the pragmas and see if buildkite complains about anything and/or if the difference is observable. I bet it's not.

libcxxabi/src/include/atomic_support.h
31

Surely defined(_LIBCPP_COMPILER_GCC) implies !defined(__clang__).

ldionne marked 8 inline comments as done.Aug 31 2021, 7:50 AM
ldionne added inline comments.
libcxx/include/__config
1249

I tried to make my patch as NFC as possible, but I agree with you.

1255–1265
libcxx/include/new
335
libcxx/include/type_traits
3517–3519
libcxx/src/include/atomic_support.h
27

Note that as a follow-up, we could also figure out whether __ATOMIC_RELAXED & friends are always defined with Clang, and if so, remove this #if #else altogether.

libcxx/src/locale.cpp
263–264

I'll try it, but if we don't see any issue, it could also be that we suppress warnings in system headers (on GCC). On Clang we do the right thing in the test suite and we surface warnings even in system headers, but not on GCC.

ldionne updated this revision to Diff 369796.Aug 31 2021, 2:07 PM
ldionne marked 6 inline comments as done.

Rebase onto main

ldionne accepted this revision.Sep 1 2021, 7:57 AM
This revision is now accepted and ready to land.Sep 1 2021, 7:57 AM