[libc++] cv-qualified types in atomic and atomic_ref (P3323R1) #121414

dalg24 · 2024-12-31T21:09:54Z

Implement P3323R1 as a DR against C++11(?), respectively C++20, for std::atomic and std::atomic_ref.

The std::atomic changes are straightforward, adding a couple static assertions checking that the class template is only instantiated for cv-unqualified types, along with a test adapted from libcxx/test/std/atomics/atomics.types.generic/trivially_copyable.verify.cpp.

The std::atomic_ref changes are more involved. I started converting a subset of the tests to add coverage for volatile- and const-qualified types but I would like feedback before I process the rest.

Implement atomic_ref related fixes from P3323 as a DR against C++20

…ed types

llvmbot · 2024-12-31T21:10:26Z

@llvm/pr-subscribers-libcxx

Author: Damien L-G (dalg24)

Changes

Implement P3323R1 as a DR against C++11(?), respectively C++20, for std::atomic and std::atomic_ref.

The std::atomic changes are straightforward, adding a couple static assertions checking that the class template is only instantiated for cv-unqualified types, along with a test adapted from libcxx/test/std/atomics/atomics.types.generic/trivially_copyable.verify.cpp.

The std::atomic_ref changes are more involved. I started converting a subset of the tests to add coverage for volatile- and const-qualified types but I would like feedback before I process the rest.

Patch is 38.03 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/121414.diff

11 Files Affected:

(modified) libcxx/docs/Status/Cxx2cPapers.csv (+1-1)
(modified) libcxx/include/__atomic/atomic_ref.h (+212-64)
(modified) libcxx/include/__atomic/support.h (+4)
(modified) libcxx/include/__cxx03/__atomic/cxx_atomic_impl.h (+4)
(modified) libcxx/test/std/atomics/atomics.ref/assign.pass.cpp (+29-13)
(modified) libcxx/test/std/atomics/atomics.ref/convert.pass.cpp (+27-7)
(modified) libcxx/test/std/atomics/atomics.ref/deduction.pass.cpp (+11-2)
(modified) libcxx/test/std/atomics/atomics.ref/load.pass.cpp (+27-8)
(modified) libcxx/test/std/atomics/atomics.ref/store.pass.cpp (+32-8)
(modified) libcxx/test/std/atomics/atomics.ref/test_helper.h (+20-19)
(added) libcxx/test/std/atomics/atomics.types.generic/cv_unqualified.verify.cpp (+25)

diff --git a/libcxx/docs/Status/Cxx2cPapers.csv b/libcxx/docs/Status/Cxx2cPapers.csv
index 3a8a794ca4ea1e..990c65a4993a3a 100644
--- a/libcxx/docs/Status/Cxx2cPapers.csv
+++ b/libcxx/docs/Status/Cxx2cPapers.csv
@@ -87,7 +87,7 @@
 "`P3050R2 <https://wg21.link/P3050R2>`__","Fix C++26 by optimizing ``linalg::conjugated`` for noncomplex value types","2024-11 (Wrocław)","","",""
 "`P3396R1 <https://wg21.link/P3396R1>`__","``std::execution`` wording fixes","2024-11 (Wrocław)","","",""
 "`P2835R7 <https://wg21.link/P2835R7>`__","Expose ``std::atomic_ref``'s object address","2024-11 (Wrocław)","","",""
-"`P3323R1 <https://wg21.link/P3323R1>`__","cv-qualified types in ``atomic`` and ``atomic_ref``","2024-11 (Wrocław)","","",""
+"`P3323R1 <https://wg21.link/P3323R1>`__","cv-qualified types in ``atomic`` and ``atomic_ref``","2024-11 (Wrocław)","|Complete|","20","Implemented as DR against C++20."
 "`P3508R0 <https://wg21.link/P3508R0>`__","Wording for ""constexpr for specialized memory algorithms""","2024-11 (Wrocław)","","",""
 "`P3369R0 <https://wg21.link/P3369R0>`__","constexpr for ``uninitialized_default_construct``","2024-11 (Wrocław)","","",""
 "`P3370R1 <https://wg21.link/P3370R1>`__","Add new library headers from C23","2024-11 (Wrocław)","","",""
diff --git a/libcxx/include/__atomic/atomic_ref.h b/libcxx/include/__atomic/atomic_ref.h
index eef15983b98331..f7ed626fed75da 100644
--- a/libcxx/include/__atomic/atomic_ref.h
+++ b/libcxx/include/__atomic/atomic_ref.h
@@ -29,7 +29,12 @@
 #include <__cstddef/ptrdiff_t.h>
 #include <__memory/addressof.h>
 #include <__type_traits/has_unique_object_representation.h>
+#include <__type_traits/is_const.h>
+#include <__type_traits/is_pointer.h>
 #include <__type_traits/is_trivially_copyable.h>
+#include <__type_traits/is_volatile.h>
+#include <__type_traits/remove_cv.h>
+#include <__type_traits/remove_pointer.h>
 #include <cstdint>
 #include <cstring>
 
@@ -110,7 +115,7 @@ struct __atomic_ref_base {
   static constexpr size_t __min_alignment = (sizeof(_Tp) & (sizeof(_Tp) - 1)) || (sizeof(_Tp) > 16) ? 0 : sizeof(_Tp);
 
 public:
-  using value_type = _Tp;
+  using value_type = __remove_cv_t<_Tp>;
 
   static constexpr size_t required_alignment = alignof(_Tp) > __min_alignment ? alignof(_Tp) : __min_alignment;
 
@@ -123,43 +128,50 @@ struct __atomic_ref_base {
 
   _LIBCPP_HIDE_FROM_ABI bool is_lock_free() const noexcept { return __atomic_is_lock_free(sizeof(_Tp), __ptr_); }
 
-  _LIBCPP_HIDE_FROM_ABI void store(_Tp __desired, memory_order __order = memory_order::seq_cst) const noexcept
-      _LIBCPP_CHECK_STORE_MEMORY_ORDER(__order) {
+  _LIBCPP_HIDE_FROM_ABI void store(value_type __desired, memory_order __order = memory_order::seq_cst) const noexcept
+    requires(!is_const_v<_Tp>)
+  _LIBCPP_CHECK_STORE_MEMORY_ORDER(__order) {
     _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(
         __order == memory_order::relaxed || __order == memory_order::release || __order == memory_order::seq_cst,
         "atomic_ref: memory order argument to atomic store operation is invalid");
     __atomic_store(__ptr_, __clear_padding(__desired), std::__to_gcc_order(__order));
   }
 
-  _LIBCPP_HIDE_FROM_ABI _Tp operator=(_Tp __desired) const noexcept {
+  _LIBCPP_HIDE_FROM_ABI value_type operator=(value_type __desired) const noexcept
+    requires(!is_const_v<_Tp>)
+  {
     store(__desired);
     return __desired;
   }
 
-  _LIBCPP_HIDE_FROM_ABI _Tp load(memory_order __order = memory_order::seq_cst) const noexcept
+  _LIBCPP_HIDE_FROM_ABI value_type load(memory_order __order = memory_order::seq_cst) const noexcept
       _LIBCPP_CHECK_LOAD_MEMORY_ORDER(__order) {
     _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(
         __order == memory_order::relaxed || __order == memory_order::consume || __order == memory_order::acquire ||
             __order == memory_order::seq_cst,
         "atomic_ref: memory order argument to atomic load operation is invalid");
-    alignas(_Tp) byte __mem[sizeof(_Tp)];
-    auto* __ret = reinterpret_cast<_Tp*>(__mem);
+    alignas(value_type) byte __mem[sizeof(value_type)];
+    auto* __ret = reinterpret_cast<value_type*>(__mem);
     __atomic_load(__ptr_, __ret, std::__to_gcc_order(__order));
     return *__ret;
   }
 
-  _LIBCPP_HIDE_FROM_ABI operator _Tp() const noexcept { return load(); }
+  _LIBCPP_HIDE_FROM_ABI operator value_type() const noexcept { return load(); }
 
-  _LIBCPP_HIDE_FROM_ABI _Tp exchange(_Tp __desired, memory_order __order = memory_order::seq_cst) const noexcept {
-    alignas(_Tp) byte __mem[sizeof(_Tp)];
-    auto* __ret = reinterpret_cast<_Tp*>(__mem);
+  _LIBCPP_HIDE_FROM_ABI value_type
+  exchange(value_type __desired, memory_order __order = memory_order::seq_cst) const noexcept
+    requires(!is_const_v<_Tp>)
+  {
+    alignas(value_type) byte __mem[sizeof(value_type)];
+    auto* __ret = reinterpret_cast<value_type*>(__mem);
     __atomic_exchange(__ptr_, __clear_padding(__desired), __ret, std::__to_gcc_order(__order));
     return *__ret;
   }
 
-  _LIBCPP_HIDE_FROM_ABI bool
-  compare_exchange_weak(_Tp& __expected, _Tp __desired, memory_order __success, memory_order __failure) const noexcept
-      _LIBCPP_CHECK_EXCHANGE_MEMORY_ORDER(__success, __failure) {
+  _LIBCPP_HIDE_FROM_ABI bool compare_exchange_weak(
+      value_type& __expected, value_type __desired, memory_order __success, memory_order __failure) const noexcept
+    requires(!is_const_v<_Tp>)
+  _LIBCPP_CHECK_EXCHANGE_MEMORY_ORDER(__success, __failure) {
     _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(
         __failure == memory_order::relaxed || __failure == memory_order::consume ||
             __failure == memory_order::acquire || __failure == memory_order::seq_cst,
@@ -172,9 +184,10 @@ struct __atomic_ref_base {
         std::__to_gcc_order(__success),
         std::__to_gcc_order(__failure));
   }
-  _LIBCPP_HIDE_FROM_ABI bool
-  compare_exchange_strong(_Tp& __expected, _Tp __desired, memory_order __success, memory_order __failure) const noexcept
-      _LIBCPP_CHECK_EXCHANGE_MEMORY_ORDER(__success, __failure) {
+  _LIBCPP_HIDE_FROM_ABI bool compare_exchange_strong(
+      value_type& __expected, value_type __desired, memory_order __success, memory_order __failure) const noexcept
+    requires(!is_const_v<_Tp>)
+  _LIBCPP_CHECK_EXCHANGE_MEMORY_ORDER(__success, __failure) {
     _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(
         __failure == memory_order::relaxed || __failure == memory_order::consume ||
             __failure == memory_order::acquire || __failure == memory_order::seq_cst,
@@ -188,8 +201,10 @@ struct __atomic_ref_base {
         std::__to_gcc_order(__failure));
   }
 
-  _LIBCPP_HIDE_FROM_ABI bool
-  compare_exchange_weak(_Tp& __expected, _Tp __desired, memory_order __order = memory_order::seq_cst) const noexcept {
+  _LIBCPP_HIDE_FROM_ABI bool compare_exchange_weak(
+      value_type& __expected, value_type __desired, memory_order __order = memory_order::seq_cst) const noexcept
+    requires(!is_const_v<_Tp>)
+  {
     return __compare_exchange(
         __ptr_,
         std::addressof(__expected),
@@ -198,8 +213,10 @@ struct __atomic_ref_base {
         std::__to_gcc_order(__order),
         std::__to_gcc_failure_order(__order));
   }
-  _LIBCPP_HIDE_FROM_ABI bool
-  compare_exchange_strong(_Tp& __expected, _Tp __desired, memory_order __order = memory_order::seq_cst) const noexcept {
+  _LIBCPP_HIDE_FROM_ABI bool compare_exchange_strong(
+      value_type& __expected, value_type __desired, memory_order __order = memory_order::seq_cst) const noexcept
+    requires(!is_const_v<_Tp>)
+  {
     return __compare_exchange(
         __ptr_,
         std::addressof(__expected),
@@ -209,7 +226,7 @@ struct __atomic_ref_base {
         std::__to_gcc_failure_order(__order));
   }
 
-  _LIBCPP_HIDE_FROM_ABI void wait(_Tp __old, memory_order __order = memory_order::seq_cst) const noexcept
+  _LIBCPP_HIDE_FROM_ABI void wait(value_type __old, memory_order __order = memory_order::seq_cst) const noexcept
       _LIBCPP_CHECK_WAIT_MEMORY_ORDER(__order) {
     _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(
         __order == memory_order::relaxed || __order == memory_order::consume || __order == memory_order::acquire ||
@@ -217,8 +234,16 @@ struct __atomic_ref_base {
         "atomic_ref: memory order argument to atomic wait operation is invalid");
     std::__atomic_wait(*this, __old, __order);
   }
-  _LIBCPP_HIDE_FROM_ABI void notify_one() const noexcept { std::__atomic_notify_one(*this); }
-  _LIBCPP_HIDE_FROM_ABI void notify_all() const noexcept { std::__atomic_notify_all(*this); }
+  _LIBCPP_HIDE_FROM_ABI void notify_one() const noexcept
+    requires(!is_const_v<_Tp>)
+  {
+    std::__atomic_notify_one(*this);
+  }
+  _LIBCPP_HIDE_FROM_ABI void notify_all() const noexcept
+    requires(!is_const_v<_Tp>)
+  {
+    std::__atomic_notify_all(*this);
+  }
 
 protected:
   using _Aligned_Tp [[__gnu__::__aligned__(required_alignment)]] = _Tp;
@@ -243,6 +268,8 @@ struct atomic_ref : public __atomic_ref_base<_Tp> {
 
   using __base = __atomic_ref_base<_Tp>;
 
+  static_assert(__base::is_always_lock_free || !is_volatile_v<_Tp>);
+
   _LIBCPP_HIDE_FROM_ABI explicit atomic_ref(_Tp& __obj) : __base(__obj) {
     _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(
         reinterpret_cast<uintptr_t>(std::addressof(__obj)) % __base::required_alignment == 0,
@@ -251,17 +278,24 @@ struct atomic_ref : public __atomic_ref_base<_Tp> {
 
   _LIBCPP_HIDE_FROM_ABI atomic_ref(const atomic_ref&) noexcept = default;
 
-  _LIBCPP_HIDE_FROM_ABI _Tp operator=(_Tp __desired) const noexcept { return __base::operator=(__desired); }
+  _LIBCPP_HIDE_FROM_ABI __base::value_type operator=(__base::value_type __desired) const noexcept
+    requires(!is_const_v<_Tp>)
+  {
+    return __base::operator=(__desired);
+  }
 
   atomic_ref& operator=(const atomic_ref&) = delete;
 };
 
 template <class _Tp>
-  requires(std::integral<_Tp> && !std::same_as<bool, _Tp>)
+  requires(std::integral<_Tp> && !std::same_as<bool, __remove_cv_t<_Tp>>)
 struct atomic_ref<_Tp> : public __atomic_ref_base<_Tp> {
   using __base = __atomic_ref_base<_Tp>;
 
+  static_assert(__base::is_always_lock_free || !is_volatile_v<_Tp>);
+
   using difference_type = __base::value_type;
+  using value_type      = __base::value_type;
 
   _LIBCPP_HIDE_FROM_ABI explicit atomic_ref(_Tp& __obj) : __base(__obj) {
     _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(
@@ -271,35 +305,90 @@ struct atomic_ref<_Tp> : public __atomic_ref_base<_Tp> {
 
   _LIBCPP_HIDE_FROM_ABI atomic_ref(const atomic_ref&) noexcept = default;
 
-  _LIBCPP_HIDE_FROM_ABI _Tp operator=(_Tp __desired) const noexcept { return __base::operator=(__desired); }
+  _LIBCPP_HIDE_FROM_ABI value_type operator=(value_type __desired) const noexcept
+    requires(!is_const_v<_Tp>)
+  {
+    return __base::operator=(__desired);
+  }
 
   atomic_ref& operator=(const atomic_ref&) = delete;
 
-  _LIBCPP_HIDE_FROM_ABI _Tp fetch_add(_Tp __arg, memory_order __order = memory_order_seq_cst) const noexcept {
+  _LIBCPP_HIDE_FROM_ABI value_type
+  fetch_add(value_type __arg, memory_order __order = memory_order_seq_cst) const noexcept
+    requires(!is_const_v<value_type>)
+  {
     return __atomic_fetch_add(this->__ptr_, __arg, std::__to_gcc_order(__order));
   }
-  _LIBCPP_HIDE_FROM_ABI _Tp fetch_sub(_Tp __arg, memory_order __order = memory_order_seq_cst) const noexcept {
+  _LIBCPP_HIDE_FROM_ABI value_type
+  fetch_sub(value_type __arg, memory_order __order = memory_order_seq_cst) const noexcept
+    requires(!is_const_v<value_type>)
+  {
     return __atomic_fetch_sub(this->__ptr_, __arg, std::__to_gcc_order(__order));
   }
-  _LIBCPP_HIDE_FROM_ABI _Tp fetch_and(_Tp __arg, memory_order __order = memory_order_seq_cst) const noexcept {
+  _LIBCPP_HIDE_FROM_ABI value_type
+  fetch_and(value_type __arg, memory_order __order = memory_order_seq_cst) const noexcept
+    requires(!is_const_v<value_type>)
+  {
     return __atomic_fetch_and(this->__ptr_, __arg, std::__to_gcc_order(__order));
   }
-  _LIBCPP_HIDE_FROM_ABI _Tp fetch_or(_Tp __arg, memory_order __order = memory_order_seq_cst) const noexcept {
+  _LIBCPP_HIDE_FROM_ABI value_type
+  fetch_or(value_type __arg, memory_order __order = memory_order_seq_cst) const noexcept
+    requires(!is_const_v<value_type>)
+  {
     return __atomic_fetch_or(this->__ptr_, __arg, std::__to_gcc_order(__order));
   }
-  _LIBCPP_HIDE_FROM_ABI _Tp fetch_xor(_Tp __arg, memory_order __order = memory_order_seq_cst) const noexcept {
+  _LIBCPP_HIDE_FROM_ABI value_type
+  fetch_xor(value_type __arg, memory_order __order = memory_order_seq_cst) const noexcept
+    requires(!is_const_v<value_type>)
+  {
     return __atomic_fetch_xor(this->__ptr_, __arg, std::__to_gcc_order(__order));
   }
 
-  _LIBCPP_HIDE_FROM_ABI _Tp operator++(int) const noexcept { return fetch_add(_Tp(1)); }
-  _LIBCPP_HIDE_FROM_ABI _Tp operator--(int) const noexcept { return fetch_sub(_Tp(1)); }
-  _LIBCPP_HIDE_FROM_ABI _Tp operator++() const noexcept { return fetch_add(_Tp(1)) + _Tp(1); }
-  _LIBCPP_HIDE_FROM_ABI _Tp operator--() const noexcept { return fetch_sub(_Tp(1)) - _Tp(1); }
-  _LIBCPP_HIDE_FROM_ABI _Tp operator+=(_Tp __arg) const noexcept { return fetch_add(__arg) + __arg; }
-  _LIBCPP_HIDE_FROM_ABI _Tp operator-=(_Tp __arg) const noexcept { return fetch_sub(__arg) - __arg; }
-  _LIBCPP_HIDE_FROM_ABI _Tp operator&=(_Tp __arg) const noexcept { return fetch_and(__arg) & __arg; }
-  _LIBCPP_HIDE_FROM_ABI _Tp operator|=(_Tp __arg) const noexcept { return fetch_or(__arg) | __arg; }
-  _LIBCPP_HIDE_FROM_ABI _Tp operator^=(_Tp __arg) const noexcept { return fetch_xor(__arg) ^ __arg; }
+  _LIBCPP_HIDE_FROM_ABI value_type operator++(int) const noexcept
+    requires(!is_const_v<_Tp>)
+  {
+    return fetch_add(value_type(1));
+  }
+  _LIBCPP_HIDE_FROM_ABI value_type operator--(int) const noexcept
+    requires(!is_const_v<_Tp>)
+  {
+    return fetch_sub(value_type(1));
+  }
+  _LIBCPP_HIDE_FROM_ABI value_type operator++() const noexcept
+    requires(!is_const_v<_Tp>)
+  {
+    return fetch_add(value_type(1)) + value_type(1);
+  }
+  _LIBCPP_HIDE_FROM_ABI value_type operator--() const noexcept
+    requires(!is_const_v<_Tp>)
+  {
+    return fetch_sub(value_type(1)) - value_type(1);
+  }
+  _LIBCPP_HIDE_FROM_ABI value_type operator+=(value_type __arg) const noexcept
+    requires(!is_const_v<_Tp>)
+  {
+    return fetch_add(__arg) + __arg;
+  }
+  _LIBCPP_HIDE_FROM_ABI value_type operator-=(value_type __arg) const noexcept
+    requires(!is_const_v<_Tp>)
+  {
+    return fetch_sub(__arg) - __arg;
+  }
+  _LIBCPP_HIDE_FROM_ABI value_type operator&=(value_type __arg) const noexcept
+    requires(!is_const_v<_Tp>)
+  {
+    return fetch_and(__arg) & __arg;
+  }
+  _LIBCPP_HIDE_FROM_ABI value_type operator|=(value_type __arg) const noexcept
+    requires(!is_const_v<_Tp>)
+  {
+    return fetch_or(__arg) | __arg;
+  }
+  _LIBCPP_HIDE_FROM_ABI value_type operator^=(value_type __arg) const noexcept
+    requires(!is_const_v<_Tp>)
+  {
+    return fetch_xor(__arg) ^ __arg;
+  }
 };
 
 template <class _Tp>
@@ -307,7 +396,10 @@ template <class _Tp>
 struct atomic_ref<_Tp> : public __atomic_ref_base<_Tp> {
   using __base = __atomic_ref_base<_Tp>;
 
+  static_assert(__base::is_always_lock_free || !is_volatile_v<_Tp>);
+
   using difference_type = __base::value_type;
+  using value_type      = __base::value_type;
 
   _LIBCPP_HIDE_FROM_ABI explicit atomic_ref(_Tp& __obj) : __base(__obj) {
     _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(
@@ -317,56 +409,112 @@ struct atomic_ref<_Tp> : public __atomic_ref_base<_Tp> {
 
   _LIBCPP_HIDE_FROM_ABI atomic_ref(const atomic_ref&) noexcept = default;
 
-  _LIBCPP_HIDE_FROM_ABI _Tp operator=(_Tp __desired) const noexcept { return __base::operator=(__desired); }
+  _LIBCPP_HIDE_FROM_ABI value_type operator=(value_type __desired) const noexcept
+    requires(!is_const_v<_Tp>)
+  {
+    return __base::operator=(__desired);
+  }
 
   atomic_ref& operator=(const atomic_ref&) = delete;
 
-  _LIBCPP_HIDE_FROM_ABI _Tp fetch_add(_Tp __arg, memory_order __order = memory_order_seq_cst) const noexcept {
-    _Tp __old = this->load(memory_order_relaxed);
-    _Tp __new = __old + __arg;
+  _LIBCPP_HIDE_FROM_ABI value_type
+  fetch_add(value_type __arg, memory_order __order = memory_order_seq_cst) const noexcept
+    requires(!is_const_v<_Tp>)
+  {
+    value_type __old = this->load(memory_order_relaxed);
+    value_type __new = __old + __arg;
     while (!this->compare_exchange_weak(__old, __new, __order, memory_order_relaxed)) {
       __new = __old + __arg;
     }
     return __old;
   }
-  _LIBCPP_HIDE_FROM_ABI _Tp fetch_sub(_Tp __arg, memory_order __order = memory_order_seq_cst) const noexcept {
-    _Tp __old = this->load(memory_order_relaxed);
-    _Tp __new = __old - __arg;
+  _LIBCPP_HIDE_FROM_ABI value_type
+  fetch_sub(value_type __arg, memory_order __order = memory_order_seq_cst) const noexcept
+    requires(!is_const_v<_Tp>)
+  {
+    value_type __old = this->load(memory_order_relaxed);
+    value_type __new = __old - __arg;
     while (!this->compare_exchange_weak(__old, __new, __order, memory_order_relaxed)) {
       __new = __old - __arg;
     }
     return __old;
   }
 
-  _LIBCPP_HIDE_FROM_ABI _Tp operator+=(_Tp __arg) const noexcept { return fetch_add(__arg) + __arg; }
-  _LIBCPP_HIDE_FROM_ABI _Tp operator-=(_Tp __arg) const noexcept { return fetch_sub(__arg) - __arg; }
+  _LIBCPP_HIDE_FROM_ABI value_type operator+=(value_type __arg) const noexcept
+    requires(!is_const_v<_Tp>)
+  {
+    return fetch_add(__arg) + __arg;
+  }
+  _LIBCPP_HIDE_FROM_ABI value_type operator-=(value_type __arg) const noexcept
+    requires(!is_const_v<_Tp>)
+  {
+    return fetch_sub(__arg) - __arg;
+  }
 };
 
 template <class _Tp>
-struct atomic_ref<_Tp*> : public __atomic_ref_base<_Tp*> {
-  using __base = __atomic_ref_base<_Tp*>;
+  requires(std::is_pointer_v<_Tp>)
+struct atomic_ref<_Tp> : public __atomic_ref_base<_Tp> {
+  using __base = __atomic_ref_base<_Tp>;
 
   using difference_type = ptrdiff_t;
+  using value_type      = typename __base::value_type;
 
-  _LIBCPP_HIDE_FROM_ABI explicit atomic_ref(_Tp*& __ptr) : __base(__ptr) {}
+  _LIBCPP_HIDE_FROM_ABI explicit atomic_ref(_Tp& __ptr) : __base(__ptr) {}
 
-  _LIBCPP_HIDE_FROM_ABI _Tp* operator=(_Tp* __desired) const noexcept { return __base::operator=(__desired); }
+  _LIBCPP_HIDE_FROM_ABI value_type operator=(value_type __desired) const noexcept
+    requires(!is_const_v<_Tp>)
+  {
+    return __base::operator=(__desired);
+  }
 
   atomic_ref& operator=(const atomic_ref&) = delete;
 
-  _LIBCPP_HIDE_FROM_ABI _Tp* fetch_add(ptrdiff_t __arg, memory_order __order = memory_order_seq_cst) const noexcept {
-    return __atomic_fetch_add(this->__ptr_, __arg * sizeof(_Tp), std::__to_gcc_order(__order));
+  _LIBCPP_HIDE_FROM_ABI value_type
+  fetch_add(ptrdiff_t __arg, memory_order __order = memory_order_seq_cst) const noexcept
+    requires(!is_const_v<_Tp>)
+  {
+    return __atomic_fetch_add(
+        this->__ptr_, __arg * sizeof(__remove_pointer_t<value_type>), std::__to_gcc_order(__order));
   }
-  _LIBCPP_HIDE_FROM_ABI _Tp* fetch_sub(ptrdiff_t __arg, memory_order __order = memory_order_seq_cst) const noexcept {
-    return __atomic_fetch_sub(this->__ptr_, __arg * sizeof(_Tp), std::__to_gcc_order(__order));
+  _LIBCPP_HIDE_FROM_ABI value_type
+  fetch_sub(ptrdiff_t __arg, memory_order __order = memory_order_seq_cst) const noexcept
+    requires(!is_const_v<_Tp>)
+  {
+    return __atomic_fetch_sub(
+        this->__ptr_, __arg * sizeof(__remove_pointer_t<value_type>), std::__to_gcc_order(__order));
   }
 
-  _LIBCPP_HIDE_FROM_ABI _Tp* operator++(int) const noexcept { return fetch_add(1); }
-  _LIBCPP_HIDE_FROM_ABI _Tp* operator--(int) const noexcept { return fetch_sub(1); }
-  _LIBCPP_HIDE_FROM_ABI _Tp* operator++() const noexcept { return fetch_add(1) + 1; }
-  _LIBCPP_HIDE_FROM_ABI _Tp* operator--() const noexcept { return fetch_sub(1) - 1; }
-  _LIBCPP_HIDE_FROM_ABI _Tp* operator+=(ptrdiff_t __arg) const noexcept { return fetch_add(__arg) + __arg; }
-  _LIBCPP_HIDE_FROM_ABI _Tp* operator-=(ptrdiff_t __arg) const noexcept { return fetch_sub(__arg) - __arg; }
+  _LIBCPP_HIDE_FROM_ABI value_type operator++(int) const noexcept
+    requires(!is_const_v<_Tp>)
+  {
+    return fetch_add(1);
+  }
+  _LIBCPP_HIDE_FROM_ABI value_type operator--(int) const noexcept
+    requires(!is_const_v<_Tp>)
+  {
+    return fetch_sub(1);
+  }
+  _LIBCPP_HIDE_FROM_ABI ...
[truncated]

github-actions · 2024-12-31T21:13:07Z

✅ With the latest revision this PR passed the C/C++ code formatter.

frederick-vs-ja

LGTM modulo tests and formatting.

libcxx/test/std/atomics/atomics.ref/store.pass.cpp

libcxx/test/std/atomics/atomics.types.generic/cv_unqualified.verify.cpp

frederick-vs-ja · 2025-01-01T14:52:01Z

libcxx/test/std/atomics/atomics.types.generic/cv_unqualified.verify.cpp

@@ -0,0 +1,25 @@
+//===----------------------------------------------------------------------===//


Perhaps there should be a test case for atomic_ref<volatile NonLockFree>.

Oh, we should also add // XFAIL: FROZEN-CXX03-HEADERS-FIXME.

…ointer to a volatile type

Adapted from libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/exchange.pass.cpp as we did for testing other functionalities. Spotted that lapse in coverage when working on llvm#121414

libcxx/test/std/atomics/atomics.ref/compare_exchange_strong.pass.cpp

Adapted from libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/exchange.pass.cpp as we did for testing other functionalities. Spotted that lapse in coverage when working on #121414.

frederick-vs-ja · 2025-01-08T03:57:49Z

libcxx/include/__atomic/atomic_ref.h

 };

 template <class _Tp>
-struct atomic_ref<_Tp*> : public __atomic_ref_base<_Tp*> {
-  using __base = __atomic_ref_base<_Tp*>;
+  requires(std::is_pointer_v<_Tp>)


Per [atomics.ref.pointer]/1, only pointer-to-object types are specially handled.

Suggested change

requires(std::is_pointer_v<_Tp>)

requires(is_pointer_v<_Tp> && is_object_v<remove_pointer_t<_Tp>>)

Also, <__type_traits/is_object.h> should be included, and std/atomics/atomics.ref/member_types.compile.pass.cpp needs to be modified.

BTW, we don't need to add std:: for non-function things unless there's conflict.

I dropped the std:: qualifier here and a couple other places.

As far as the pointer-to-object comment is concerned, I would suggest we open a separate issue for it and treat it as a defect in the current implementation. Do you agree?

FWIW, libstdc++ has the same issue and it looks like STL got it right https://godbolt.org/z/843vss7na

As far as the pointer-to-object comment is concerned, I would suggest we open a separate issue for it and treat it as a defect in the current implementation. Do you agree?

I haven't understood why a separate issue is wanted. I think this is a design change in P3323R1, although it should be DR-backed to C++20.

The current "latest" version of MSVC STL on Compiler Explorer hasn't got microsoft/STL#4689 merged yet. You can find that _MSVC_STL_UPDATE was defined as 202404L in that version, but the PR was merged later. After microsoft/STL#4689, MSVC STL also became "wrong", which was conforming to the standard not amended by P3323R1.

Although the status quo is somehow inconsitent - atomic<void*> should have difference_type while atomic_ref<void*> should not.

CC @gonzalobg.

I misunderstood. I was under the impression that the C++20 spec had the pointer-to-object mention but I see now your point that this was introduced by P3323. I will follow your suggestion and update the tests as needed. Sorry about the confusion.

Thanks for the ping. Agree with the inconsistency. Will get back to you.

libcxx/test/std/atomics/atomics.ref/increment_decrement.pass.cpp

libcxx/include/__cxx03/__atomic/cxx_atomic_impl.h

huixie90 · 2025-01-12T12:01:11Z

libcxx/include/__atomic/atomic_ref.h

@@ -29,7 +29,13 @@
 #include <__cstddef/ptrdiff_t.h>
 #include <__memory/addressof.h>
 #include <__type_traits/has_unique_object_representation.h>
+#include <__type_traits/is_const.h>


sorry just a question:
I did not follow the paper but do you know what is the reason to support atomic_ref<const Foo>. One possible use case I can think of is something like

int val = 0; // writer thread atomic_ref<int> a(val); a.store(...); // reader thread atomic_ref<const int> a(val); a.load(...);

Or, did I misunderstand anything? Could you please add a test for the use case that is designed for? (Currently I only see a test for const T in load, where it inits a variable and load it. which is a must have test case, but I guess it does not represent the real use case it is designed for)

One possible use case I can think of is something like

This is possibly the only practical use of atomic_ref<const Foo> (search results).
But it doesn't seem present in libc++'s test suit. Agree on that we should test it.

I agree with the sentiment that this needs a test. However, based on my discussion with @dalg24 just now, I fail to see how the test helper can be modified to accommodate that. Given that, I think it's acceptable to move forward with the patch despite not having great "runtime behavior" coverage for atomic_ref<const T>.

If you have ideas for changing the test driver, please let us know.

…ic_ref_cv_qualified_types_p3323

libcxx/docs/Status/Cxx2cPapers.csv

libcxx/include/__atomic/support.h

ldionne · 2025-01-31T19:52:44Z

libcxx/test/std/atomics/atomics.types.generic/cv_unqualified.verify.cpp

@@ -0,0 +1,25 @@
+//===----------------------------------------------------------------------===//


ldionne · 2025-01-31T20:07:00Z

libcxx/include/__atomic/atomic_ref.h

  using difference_type = __base::value_type;
+  using value_type      = __base::value_type;


I would get rid of this one and the one for floating_point since this is already provided in __atomic_ref_impl. This makes it clearer that we're only providing the typedefs that we actually need to fulfill the public API, which in this case is difference_type.

If you have trouble referring to value_type (I think you might need something like typename atomic_ref::value_type which is annoying), you could always do

using typename __base::value_type;

IIRC that's legal and that makes it clear that we're only bringing the name that already exists into the current scope instead of introducing a new name.

ldionne · 2025-01-31T20:11:09Z

libcxx/include/__atomic/atomic_ref.h

 struct atomic_ref<_Tp> : public __atomic_ref_base<_Tp> {
  using __base _LIBCPP_NODEBUG = __atomic_ref_base<_Tp>;

+  static_assert(__base::is_always_lock_free || !is_volatile_v<_Tp>);


Here and above, let's add a short error message. We can maybe say that the Standard disallows this along with a rationale if there's one. As-is, users could be left thinking that it's an artifact of our implementation or something. Let's be explicit about the fact that the Standard doesn't allow it.

libcxx/test/std/atomics/atomics.ref/assign.pass.cpp

…ic_ref_cv_qualified_types_p3323

ldionne · 2025-03-31T19:29:33Z

libcxx/test/std/atomics/atomics.ref/test_helper.h


-    T copy_x = old_value;
+    T copy_x = const_cast<std::remove_cv_t<T> const&>(old_value);


I don't understand why this needs a const_cast, can you explain?

ldionne · 2025-03-31T19:34:18Z

libcxx/test/std/atomics/atomics.ref/test_helper.h

@@ -45,12 +45,12 @@ template <class T, class StoreOp, class LoadOp>
 void test_seq_cst(StoreOp store_op, LoadOp load_op) {


Should we introduce using Unqualified = std::remove_cv_t<T> here like we do in other places?

ldionne · 2025-03-31T19:34:51Z

libcxx/test/std/atomics/atomics.ref/test_helper.h

+    T old_value(make_value<std::remove_cv_t<T>>(0));
+    T new_value(make_value<std::remove_cv_t<T>>(1));


Suggested change

T old_value(make_value<std::remove_cv_t<T>>(0));

T new_value(make_value<std::remove_cv_t<T>>(1));

Unqualified old_value(make_value<Unqualified>(0));

Unqualified new_value(make_value<Unqualified>(1));

ldionne · 2025-03-31T19:49:53Z

libcxx/include/__atomic/atomic_ref.h

@@ -29,7 +29,13 @@
 #include <__cstddef/ptrdiff_t.h>
 #include <__memory/addressof.h>
 #include <__type_traits/has_unique_object_representation.h>
+#include <__type_traits/is_const.h>


I agree with the sentiment that this needs a test. However, based on my discussion with @dalg24 just now, I fail to see how the test helper can be modified to accommodate that. Given that, I think it's acceptable to move forward with the patch despite not having great "runtime behavior" coverage for atomic_ref<const T>.

If you have ideas for changing the test driver, please let us know.

ldionne · 2025-03-31T19:50:47Z

libcxx/test/std/atomics/atomics.ref/load.pass.cpp

-      test_seq_cst<T>(store, load_no_arg);
-      test_seq_cst<T>(store, load_with_order);
-    }
+    if constexpr (!std::is_const_v<T>) { // FIXME


You can change the comment to a short explanation of why we can't test that, something like:

// The test helper needs the ability to store to the atomic_ref, which doesn't work for a const T.

…pyable mandates (#131754) 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`)

dalg24 added 4 commits December 31, 2024 14:38

[libc++] cv-qualified types in atomic_ref

5afe474

Implement atomic_ref related fixes from P3323 as a DR against C++20

[libc++] check that std::atomic is only instantiated for cv-unqualifi…

fc886c3

…ed types

[libc++] Mark P3323R1 as complete

69d92fa

[libc++] Add testing for atomic_ref store/load for cv-qualified types

9c6dd66

dalg24 requested a review from a team as a code owner December 31, 2024 21:09

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Dec 31, 2024

Trim trailing empty line in test to please clang-format

1077d03

frederick-vs-ja reviewed Jan 1, 2025

View reviewed changes

This comment was marked as resolved.

Sign in to view

dalg24 added 3 commits January 2, 2025 08:06

Add volatile T test for std::atomic

28a0107

Fix GCC error about 2nd argument to the '__atomic' builtins being a p…

26013d0

…ointer to a volatile type

Convert compare_exchange_strong test

ffdda72

dalg24 mentioned this pull request Jan 3, 2025

[libc++] Improve coverage of std::atomic_ref<T>::exchange() #121596

Merged

frederick-vs-ja reviewed Jan 4, 2025

View reviewed changes

libcxx/test/std/atomics/atomics.ref/compare_exchange_strong.pass.cpp Outdated Show resolved Hide resolved

Prefer parameter list in requires-expression to avoid declval

1b24aeb

dalg24 force-pushed the atomic_ref_cv_qualified_types_p3323 branch from 2b782ab to 1b24aeb Compare January 7, 2025 23:30

frederick-vs-ja requested changes Jan 8, 2025

View reviewed changes

Drop unnecessary std:: qualifiers

a2f7780

huixie90 assigned huixie90 and unassigned huixie90 Jan 9, 2025

dalg24 added 2 commits January 9, 2025 07:12

Fix constraint on specialization for pointer-to-object types

9ec6b04

Update tests for pointer types such as void*

d878901

dalg24 commented Jan 9, 2025

View reviewed changes

libcxx/test/std/atomics/atomics.ref/increment_decrement.pass.cpp Outdated Show resolved Hide resolved

huixie90 reviewed Jan 12, 2025

View reviewed changes

Undo cxx03 changes

f350f7f

Merge branch 'main' of https://github.com/llvm/llvm-project into atom…

b156123

…ic_ref_cv_qualified_types_p3323

ldionne requested changes Jan 31, 2025

View reviewed changes

dalg24 added 6 commits January 31, 2025 21:47

Merge branch 'main' of https://github.com/llvm/llvm-project into atom…

e0fd1a2

…ic_ref_cv_qualified_types_p3323

Add a release note

0b96b4c

Prefer not is_const and not is_volatile when checking cv-unqualified

dcbee5b

Prefer using-declaration for value_type in derived classes

232a8a5

Merge branch 'main' of https://github.com/llvm/llvm-project into atom…

d84ec1f

…ic_ref_cv_qualified_types_p3323

Follow pattern suggested by Louis in the tests

afe849f

dalg24 mentioned this pull request Mar 18, 2025

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

Merged

Merge branch 'main' into atomic_ref_cv_qualified_types_p3323

b8a4edc

ldionne reviewed Mar 31, 2025

View reviewed changes

		@@ -0,0 +1,25 @@
		//===----------------------------------------------------------------------===//

	requires(std::is_pointer_v<_Tp>)
	requires(is_pointer_v<_Tp> && is_object_v<remove_pointer_t<_Tp>>)

		using difference_type = __base::value_type;
		using value_type = __base::value_type;


		T copy_x = old_value;
		T copy_x = const_cast<std::remove_cv_t<T> const&>(old_value);

		@@ -45,12 +45,12 @@ template <class T, class StoreOp, class LoadOp>
		void test_seq_cst(StoreOp store_op, LoadOp load_op) {

		T old_value(make_value<std::remove_cv_t<T>>(0));
		T new_value(make_value<std::remove_cv_t<T>>(1));

[libc++] cv-qualified types in atomic and atomic_ref (P3323R1) #121414

Are you sure you want to change the base?

[libc++] cv-qualified types in atomic and atomic_ref (P3323R1) #121414

Uh oh!

Conversation

dalg24 commented Dec 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Dec 31, 2024

Uh oh!

github-actions bot commented Dec 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

frederick-vs-ja left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frederick-vs-ja Jan 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dalg24 commented Dec 31, 2024 •

edited

Loading

github-actions bot commented Dec 31, 2024 •

edited

Loading

frederick-vs-ja Jan 9, 2025 •

edited

Loading