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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone 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__))
----------------
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__`.


================
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__)
----------------
Let's merge these two #ifs.


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


================
Comment at: libcxx/include/type_traits:3128
 
-#if __has_feature(is_trivially_constructible) || _GNUC_VER >= 501
+#if __has_feature(is_trivially_constructible) || defined(_LIBCPP_COMPILER_GCC)
 
----------------
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?


================
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
----------------
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".


================
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
----------------
Surely `defined(_LIBCPP_COMPILER_GCC)` already implies `!defined(__clang__)`.


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


================
Comment at: libcxxabi/src/include/atomic_support.h:31
 #   define _LIBCXXABI_HAS_ATOMIC_BUILTINS
-#elif !defined(__clang__) && defined(_GNUC_VER) && _GNUC_VER >= 407
+#elif !defined(__clang__) && defined(_LIBCPP_COMPILER_GCC)
 #   define _LIBCXXABI_HAS_ATOMIC_BUILTINS
----------------
Surely `defined(_LIBCPP_COMPILER_GCC)` implies `!defined(__clang__)`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108923