[libcxx-commits] [PATCH] D108923: [libc++] Remove workarounds for unsupported GCC and Clang versions

[Louis Dionne via Phabricator via libcxx-commits]

ldionne marked 8 inline comments as done.
ldionne added inline comments.


================
Comment at: libcxx/include/__config:1251
 #  define _LIBCPP_FALLTHROUGH() [[clang::fallthrough]]
-#elif __has_attribute(fallthrough) || _GNUC_VER >= 700
+#elif __has_attribute(fallthrough) || defined(_LIBCPP_COMPILER_GCC)
 #  define _LIBCPP_FALLTHROUGH() __attribute__((__fallthrough__))
----------------
Quuxplusone wrote:
> 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__`.
I tried to make my patch as NFC as possible, but I agree with you.


================
Comment at: libcxx/include/__config:1257-1267
 #if __has_attribute(__nodebug__)
 #define _LIBCPP_NODEBUG __attribute__((__nodebug__))
 #else
 #define _LIBCPP_NODEBUG
 #endif
 
+#if __has_attribute(__nodebug__)
----------------
Quuxplusone wrote:
> Let's merge these two #ifs.
D108996


================
Comment at: libcxx/include/new:335
 template <class _Tp>
 _LIBCPP_NODISCARD_AFTER_CXX17 inline
 _LIBCPP_CONSTEXPR _Tp* __launder(_Tp* __p) _NOEXCEPT
----------------
Quuxplusone wrote:
> Is this function template missing `_LIBCPP_HIDE_FROM_ABI` / `_LIBCPP_INLINE_VISIBILITY`?
e79474d337c62de7d3efe7fc2504ade46eda10e8


================
Comment at: libcxx/include/type_traits:3558-3565
 template <class _Tp> struct _LIBCPP_TEMPLATE_VIS is_trivially_copyable
-#if __has_feature(is_trivially_copyable)
-    : public integral_constant<bool, __is_trivially_copyable(_Tp)>
-#elif _GNUC_VER >= 501
+// GCC has not implemented Core 2094 which makes volatile qualified types trivially copyable.
+#if defined(_LIBCPP_COMPILER_GCC)
     : public integral_constant<bool, !is_volatile<_Tp>::value && __is_trivially_copyable(_Tp)>
-#else
-    : integral_constant<bool, is_scalar<typename remove_all_extents<_Tp>::type>::value>
+#else __has_feature(is_trivially_copyable)
+    : public integral_constant<bool, __is_trivially_copyable(_Tp)>
 #endif
----------------
Quuxplusone wrote:
> 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".
D108997


================
Comment at: libcxx/src/include/atomic_support.h:27
 #   define _LIBCPP_HAS_ATOMIC_BUILTINS
-#elif !defined(__clang__) && defined(_GNUC_VER) && _GNUC_VER >= 407
+#elif !defined(__clang__) && defined(_LIBCPP_COMPILER_GCC)
 #   define _LIBCPP_HAS_ATOMIC_BUILTINS
----------------
Quuxplusone wrote:
> Surely `defined(_LIBCPP_COMPILER_GCC)` already implies `!defined(__clang__)`.
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.


================
Comment at: libcxx/src/locale.cpp:267
 #pragma GCC diagnostic push
 #pragma GCC diagnostic ignored "-Wextra"
 
----------------
Quuxplusone wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108923/new/

https://reviews.llvm.org/D108923