[libc++] Improve diagnostic when violating std::atomic trivially copyable mandates

[dalg24]

When attempting to instantiate std::atomic with a non trivially copyable type, one gets errors from instantiating internals before the actual static assertion that check the template parameter type requirements.

The verify test for it had a // ADDITIONAL_COMPILE_FLAGS: -Xclang -verify-ignore-unexpected=error directive to work around this issue. The changes I propose enable us to drop that directive.
As I understand it, the verify test was misplaced so I moved it to test/{std -> libcxx}/atomics.

(I ran into this while working on #121414 in which we would add another static assertion in __check_atomic_mandates)

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Damien L-G (dalg24)

Changes

When attempting to instantiate std::atomic with a non trivially copyable type, one gets errors from instantiating internals before the actual static assertion that check the template parameter type requirements.

The verify test for it had a // ADDITIONAL_COMPILE_FLAGS: -Xclang -verify-ignore-unexpected=error directive to work around this issue. The changes I propose enable us to drop that directive.
As I understand it, the verify test was misplaced so I moved it to test/{std -> libcxx}/atomics.

(I ran into this while working on #121414 in which we would add another static assertion in __check_atomic_mandates)

---

Full diff: https://github.com/llvm/llvm-project/pull/131754.diff

3 Files Affected:

• (modified) libcxx/include/__atomic/support.h (+6-2)
• (added) libcxx/test/libcxx/atomics/atomics.types.generic/trivially_copyable.verify.cpp (+26)
• (removed) libcxx/test/std/atomics/atomics.types.generic/trivially_copyable.verify.cpp (-31)

diff --git a/libcxx/include/__atomic/support.h b/libcxx/include/__atomic/support.h
index 4b555ab483ca0..232ee23ea1db2 100644
--- a/libcxx/include/__atomic/support.h
+++ b/libcxx/include/__atomic/support.h
@@ -111,10 +111,14 @@
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
-template <typename _Tp, typename _Base = __cxx_atomic_base_impl<_Tp> >
-struct __cxx_atomic_impl : public _Base {
+template <typename _Tp>
+struct __check_atomic_mandates {
+  using type = _Tp;
   static_assert(is_trivially_copyable<_Tp>::value, "std::atomic<T> requires that 'T' be a trivially copyable type");
+};
 
+template <typename _Tp, typename _Base = __cxx_atomic_base_impl<typename __check_atomic_mandates<_Tp>::type> >
+struct __cxx_atomic_impl : public _Base {
   _LIBCPP_HIDE_FROM_ABI __cxx_atomic_impl() _NOEXCEPT = default;
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR explicit __cxx_atomic_impl(_Tp __value) _NOEXCEPT : _Base(__value) {}
 };
diff --git a/libcxx/test/libcxx/atomics/atomics.types.generic/trivially_copyable.verify.cpp b/libcxx/test/libcxx/atomics/atomics.types.generic/trivially_copyable.verify.cpp
new file mode 100644
index 0000000000000..c869e1fe76841
--- /dev/null
+++ b/libcxx/test/libcxx/atomics/atomics.types.generic/trivially_copyable.verify.cpp
@@ -0,0 +1,26 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// <atomic>
+
+// template <class T>
+// struct atomic;
+
+#include <atomic>
+
+struct NotTriviallyCopyable {
+  explicit NotTriviallyCopyable(int i) : i_(i) {}
+  NotTriviallyCopyable(const NotTriviallyCopyable& rhs) : i_(rhs.i_) {}
+  int i_;
+};
+
+void f() {
+  NotTriviallyCopyable x(42);
+  std::atomic<NotTriviallyCopyable> a(
+      x); // expected-error@*:* {{std::atomic<T> requires that 'T' be a trivially copyable type}}
+}
diff --git a/libcxx/test/std/atomics/atomics.types.generic/trivially_copyable.verify.cpp b/libcxx/test/std/atomics/atomics.types.generic/trivially_copyable.verify.cpp
deleted file mode 100644
index 0955707cdcf38..0000000000000
--- a/libcxx/test/std/atomics/atomics.types.generic/trivially_copyable.verify.cpp
+++ /dev/null
@@ -1,31 +0,0 @@
-//===----------------------------------------------------------------------===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-// <atomic>
-
-// template <class T>
-// struct atomic;
-
-// This test checks that we static_assert inside std::atomic<T> when T
-// is not trivially copyable, however Clang will sometimes emit additional
-// errors while trying to instantiate the rest of std::atomic<T>.
-// We silence those to make the test more robust.
-// ADDITIONAL_COMPILE_FLAGS: -Xclang -verify-ignore-unexpected=error
-
-#include <atomic>
-
-struct NotTriviallyCopyable {
-  explicit NotTriviallyCopyable(int i) : i_(i) { }
-  NotTriviallyCopyable(const NotTriviallyCopyable &rhs) : i_(rhs.i_) { }
-  int i_;
-};
-
-void f() {
-  NotTriviallyCopyable x(42);
-  std::atomic<NotTriviallyCopyable> a(x); // expected-error@*:* {{std::atomic<T> requires that 'T' be a trivially copyable type}}
-}

[dalg24]

In case you wonder what the status quo looks like https://godbolt.org/z/TdexohhcM

[ldionne]

Thanks for the patch. This is a good improvement, and the fact that the test would previously be equivalent to a .compile.fail.cpp was really surprising to me.

Looks like we need to eradicate -Xclang -verify-ignore-unexpected=error from our verify tests.