[libc++] std::atomic primary template should not have a `difference_type` member type #123236

dalg24 · 2025-01-16T20:17:31Z

The test would not check its absence and the code path intended for pointer was never actually instantiated.
I added a few pointer types since there was no coverage.

llvmbot · 2025-01-16T20:18:08Z

@llvm/pr-subscribers-libcxx

Author: Damien L-G (dalg24)

Changes

The test would not check its absence and the code path intended for pointer was never actually instantiated.
I added a few pointer types since there was no coverage.

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

2 Files Affected:

(modified) libcxx/include/__atomic/atomic.h (+6-4)
(modified) libcxx/test/std/atomics/types.pass.cpp (+115-105)

diff --git a/libcxx/include/__atomic/atomic.h b/libcxx/include/__atomic/atomic.h
index 975a479e204004..1ed206ab2ff197 100644
--- a/libcxx/include/__atomic/atomic.h
+++ b/libcxx/include/__atomic/atomic.h
@@ -40,6 +40,8 @@ struct __atomic_base // false
 {
   mutable __cxx_atomic_impl<_Tp> __a_;
 
+  using value_type = _Tp;
+
 #if _LIBCPP_STD_VER >= 17
   static constexpr bool is_always_lock_free = __libcpp_is_always_lock_free<__cxx_atomic_impl<_Tp> >::__value;
 #endif
@@ -145,6 +147,8 @@ template <class _Tp>
 struct __atomic_base<_Tp, true> : public __atomic_base<_Tp, false> {
   using __base _LIBCPP_NODEBUG = __atomic_base<_Tp, false>;
 
+  using difference_type = __base::value_type;
+
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 __atomic_base() _NOEXCEPT = default;
 
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR __atomic_base(_Tp __d) _NOEXCEPT : __base(__d) {}
@@ -229,8 +233,6 @@ struct __atomic_waitable_traits<__atomic_base<_Tp, _IsIntegral> > {
 template <class _Tp>
 struct atomic : public __atomic_base<_Tp> {
   using __base _LIBCPP_NODEBUG = __atomic_base<_Tp>;
-  using value_type             = _Tp;
-  using difference_type        = value_type;
 
 #if _LIBCPP_STD_VER >= 20
   _LIBCPP_HIDE_FROM_ABI atomic() = default;
@@ -258,8 +260,8 @@ struct atomic : public __atomic_base<_Tp> {
 template <class _Tp>
 struct atomic<_Tp*> : public __atomic_base<_Tp*> {
   using __base _LIBCPP_NODEBUG = __atomic_base<_Tp*>;
-  using value_type             = _Tp*;
-  using difference_type        = ptrdiff_t;
+
+  using difference_type = ptrdiff_t;
 
   _LIBCPP_HIDE_FROM_ABI atomic() _NOEXCEPT = default;
 
diff --git a/libcxx/test/std/atomics/types.pass.cpp b/libcxx/test/std/atomics/types.pass.cpp
index 33512c037804f8..dd8f770ac45434 100644
--- a/libcxx/test/std/atomics/types.pass.cpp
+++ b/libcxx/test/std/atomics/types.pass.cpp
@@ -29,163 +29,173 @@
 #  include <thread>
 #endif
 
-template <class A, bool Integral>
-struct test_atomic
-{
-    test_atomic()
-    {
-        A a; (void)a;
+// detect existence of the difference_type member type
+template <class...>
+using myvoid_t = void;
+template <typename T, typename = void>
+struct has_difference_type : std::false_type {};
+template <typename T>
+struct has_difference_type<T, myvoid_t<typename T::difference_type> > : std::true_type {};
+
+template <class A, bool IntegralOrFloating, bool Pointer>
+struct test_atomic {
+  test_atomic() {
+    static_assert(!IntegralOrFloating || !Pointer, "");
+    A a;
+    (void)a;
 #if TEST_STD_VER >= 17
     static_assert((std::is_same_v<typename A::value_type, decltype(a.load())>), "");
+    static_assert(!has_difference_type<A>::value, "");
 #endif
-    }
+  }
 };
 
 template <class A>
-struct test_atomic<A, true>
-{
-    test_atomic()
-    {
-        A a; (void)a;
+struct test_atomic<A, true, false> {
+  test_atomic() {
+    A a;
+    (void)a;
 #if TEST_STD_VER >= 17
     static_assert((std::is_same_v<typename A::value_type, decltype(a.load())>), "");
     static_assert((std::is_same_v<typename A::value_type, typename A::difference_type>), "");
 #endif
-    }
+  }
 };
 
 template <class A>
-struct test_atomic<A*, false>
-{
-    test_atomic()
-    {
-        A a; (void)a;
+struct test_atomic<A, false, true> {
+  test_atomic() {
+    A a;
+    (void)a;
 #if TEST_STD_VER >= 17
     static_assert((std::is_same_v<typename A::value_type, decltype(a.load())>), "");
     static_assert((std::is_same_v<typename A::difference_type, std::ptrdiff_t>), "");
 #endif
-    }
+  }
 };
 
 template <class T>
-void
-test()
-{
-    using A = std::atomic<T>;
+void test() {
+  using A = std::atomic<T>;
 #if TEST_STD_VER >= 17
-    static_assert((std::is_same_v<typename A::value_type, T>), "");
+  static_assert((std::is_same_v<typename A::value_type, T>), "");
 #endif
-    test_atomic<A, std::is_integral<T>::value && !std::is_same<T, bool>::value>();
+  constexpr bool IntegralOrFloating =
+      (std::is_integral<T>::value && !std::is_same<T, bool>::value) || std::is_floating_point<T>::value;
+  constexpr bool Pointer = std::is_pointer<T>::value;
+  test_atomic<A, IntegralOrFloating, Pointer>();
 }
 
 struct TriviallyCopyable {
-    int i_;
+  int i_;
 };
 
-struct WeirdTriviallyCopyable
-{
-    char i, j, k; /* the 3 chars of doom */
+struct WeirdTriviallyCopyable {
+  char i, j, k; /* the 3 chars of doom */
 };
 
-struct PaddedTriviallyCopyable
-{
-    char i; int j; /* probably lock-free? */
+struct PaddedTriviallyCopyable {
+  char i;
+  int j; /* probably lock-free? */
 };
 
-struct LargeTriviallyCopyable
-{
-    int i, j[127]; /* decidedly not lock-free */
+struct LargeTriviallyCopyable {
+  int i, j[127]; /* decidedly not lock-free */
 };
 
-int main(int, char**)
-{
-    test<bool>               ();
-    test<char>               ();
-    test<signed char>        ();
-    test<unsigned char>      ();
-    test<short>              ();
-    test<unsigned short>     ();
-    test<int>                ();
-    test<unsigned int>       ();
-    test<long>               ();
-    test<unsigned long>      ();
-    test<long long>          ();
-    test<unsigned long long> ();
+int main(int, char**) {
+  test<bool>();
+  test<char>();
+  test<signed char>();
+  test<unsigned char>();
+  test<short>();
+  test<unsigned short>();
+  test<int>();
+  test<unsigned int>();
+  test<long>();
+  test<unsigned long>();
+  test<long long>();
+  test<unsigned long long>();
 #if TEST_STD_VER > 17 && defined(__cpp_char8_t)
-    test<char8_t>            ();
+  test<char8_t>();
 #endif
-    test<char16_t>           ();
-    test<char32_t>           ();
+  test<char16_t>();
+  test<char32_t>();
 #ifndef TEST_HAS_NO_WIDE_CHARACTERS
-    test<wchar_t>            ();
+  test<wchar_t>();
 #endif
 
-    test<std::int_least8_t>   ();
-    test<std::uint_least8_t>  ();
-    test<std::int_least16_t>  ();
-    test<std::uint_least16_t> ();
-    test<std::int_least32_t>  ();
-    test<std::uint_least32_t> ();
-    test<std::int_least64_t>  ();
-    test<std::uint_least64_t> ();
-
-    test<std::int_fast8_t>   ();
-    test<std::uint_fast8_t>  ();
-    test<std::int_fast16_t>  ();
-    test<std::uint_fast16_t> ();
-    test<std::int_fast32_t>  ();
-    test<std::uint_fast32_t> ();
-    test<std::int_fast64_t>  ();
-    test<std::uint_fast64_t> ();
-
-    test< std::int8_t>  ();
-    test<std::uint8_t>  ();
-    test< std::int16_t> ();
-    test<std::uint16_t> ();
-    test< std::int32_t> ();
-    test<std::uint32_t> ();
-    test< std::int64_t> ();
-    test<std::uint64_t> ();
-
-    test<std::intptr_t>  ();
-    test<std::uintptr_t> ();
-    test<std::size_t>    ();
-    test<std::ptrdiff_t> ();
-    test<std::intmax_t>  ();
-    test<std::uintmax_t> ();
-
-    test<std::uintmax_t> ();
-    test<std::uintmax_t> ();
-
-    test<TriviallyCopyable>();
-    test<PaddedTriviallyCopyable>();
+  test<std::int_least8_t>();
+  test<std::uint_least8_t>();
+  test<std::int_least16_t>();
+  test<std::uint_least16_t>();
+  test<std::int_least32_t>();
+  test<std::uint_least32_t>();
+  test<std::int_least64_t>();
+  test<std::uint_least64_t>();
+
+  test<std::int_fast8_t>();
+  test<std::uint_fast8_t>();
+  test<std::int_fast16_t>();
+  test<std::uint_fast16_t>();
+  test<std::int_fast32_t>();
+  test<std::uint_fast32_t>();
+  test<std::int_fast64_t>();
+  test<std::uint_fast64_t>();
+
+  test< std::int8_t>();
+  test<std::uint8_t>();
+  test< std::int16_t>();
+  test<std::uint16_t>();
+  test< std::int32_t>();
+  test<std::uint32_t>();
+  test< std::int64_t>();
+  test<std::uint64_t>();
+
+  test<std::intptr_t>();
+  test<std::uintptr_t>();
+  test<std::size_t>();
+  test<std::ptrdiff_t>();
+  test<std::intmax_t>();
+  test<std::uintmax_t>();
+
+  test<std::uintmax_t>();
+  test<std::uintmax_t>();
+
+  test<void*>();
+  test<const void*>();
+  test<int*>();
+  test<const int*>();
+
+  test<TriviallyCopyable>();
+  test<PaddedTriviallyCopyable>();
 #ifndef __APPLE__ // Apple doesn't ship libatomic
-    /*
+  /*
         These aren't going to be lock-free,
         so some libatomic.a is necessary.
     */
-    test<WeirdTriviallyCopyable>();
-    test<LargeTriviallyCopyable>();
+  test<WeirdTriviallyCopyable>();
+  test<LargeTriviallyCopyable>();
 #endif
 
 #ifndef TEST_HAS_NO_THREADS
-    test<std::thread::id>();
+  test<std::thread::id>();
 #endif
-    test<std::chrono::nanoseconds>();
-    test<float>();
+  test<std::chrono::nanoseconds>();
+  test<float>();
 
 #if TEST_STD_VER >= 20
-    test<std::atomic_signed_lock_free::value_type>();
-    static_assert(std::is_signed_v<std::atomic_signed_lock_free::value_type>);
-    static_assert(std::is_integral_v<std::atomic_signed_lock_free::value_type>);
+  test<std::atomic_signed_lock_free::value_type>();
+  static_assert(std::is_signed_v<std::atomic_signed_lock_free::value_type>);
+  static_assert(std::is_integral_v<std::atomic_signed_lock_free::value_type>);
 
-    test<std::atomic_unsigned_lock_free::value_type>();
-    static_assert(std::is_unsigned_v<std::atomic_unsigned_lock_free::value_type>);
-    static_assert(std::is_integral_v<std::atomic_unsigned_lock_free::value_type>);
+  test<std::atomic_unsigned_lock_free::value_type>();
+  static_assert(std::is_unsigned_v<std::atomic_unsigned_lock_free::value_type>);
+  static_assert(std::is_integral_v<std::atomic_unsigned_lock_free::value_type>);
 /*
     test<std::shared_ptr<int>>();
 */
 #endif
 
-    return 0;
+  return 0;
 }

dalg24 · 2025-01-16T20:19:26Z

libcxx/test/std/atomics/types.pass.cpp

 };

 template <class A>
-struct test_atomic<A*, false>


This would never occur since we were instantiating with A = std::atomic<T>

dalg24 · 2025-01-16T20:27:36Z

FWIW it looks like it was only broken in libc++ https://godbolt.org/z/x8G5qqvzW

philnik777

The implementation changes look good, but I haven't looked closely at the test yet.

libcxx/test/std/atomics/types.pass.cpp

…ters to member function

Work around `error: unknown type name 'constexpr'`. Not sure that is the right approach since `<atomic>` is C++11...

dalg24 · 2025-01-17T11:52:06Z

The implementation changes look good, but I haven't looked closely at the test yet.

I could use directions on how to approach the cxx03 failures?
<atomic> is C++11. Should I just add // UNSUPPORTED: c++03 directives where I got failures?

ldionne · 2025-01-20T14:03:12Z

The implementation changes look good, but I haven't looked closely at the test yet.

I could use directions on how to approach the cxx03 failures? <atomic> is C++11. Should I just add // UNSUPPORTED: c++03 directives where I got failures?

For that test you should add // XFAIL: FROZEN-CXX03-HEADERS-FIXME

libcxx/test/std/atomics/types.pass.cpp

…ic_difference_type

Drop guards for C++ version greater equal 17 to increase coverage.

libcxx/test/std/atomics/types.pass.cpp

dalg24 requested a review from a team as a code owner January 16, 2025 20:17

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Jan 16, 2025

dalg24 commented Jan 16, 2025

View reviewed changes

dalg24 changed the title ~~[libc++] std::atomic primary template should not have a difference type member type~~ [libc++] std::atomic primary template should not have a difference_type member type Jan 16, 2025

philnik777 reviewed Jan 16, 2025

View reviewed changes

libcxx/test/std/atomics/types.pass.cpp Show resolved Hide resolved

dalg24 added 5 commits January 16, 2025 16:39

Apply clang-format

3695367

Fix tests for std::atomic member types

857e15f

Fix std::atomic primary template should not have difference_type

c1ea966

Update expected error for atomic_fetch_{add,sub}[_explicit] with poin…

af4d5f5

…ters to member function

Add test with function pointer

95a84d4

dalg24 force-pushed the atomic_difference_type branch from 11739d3 to 95a84d4 Compare January 16, 2025 21:47

Fixup for cxx03

145d45a

Work around `error: unknown type name 'constexpr'`. Not sure that is the right approach since `<atomic>` is C++11...

ldionne requested changes Jan 20, 2025

View reviewed changes

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

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

dalg24 added 2 commits January 21, 2025 09:44

Apply suggested changes from review

163a79a

Add XFAIL directive for the frozen cxx03 tests per review

64257b4

ldionne approved these changes Jan 27, 2025

View reviewed changes

ldionne added the pending-ci Merging the PR is only pending completion of CI label Jan 27, 2025

dalg24 and others added 5 commits January 29, 2025 18:41

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

fc2dc57

…ic_difference_type

Fixup missing typename keyword

f16cd7b

Fixup std::atomic<Floating>::difference_type was introduced in C++20

bacb6bb

Drop guards for C++ version greater equal 17 to increase coverage.

Merge branch 'main' into atomic_difference_type

fbaebba

XFAIL test in C++03 frozen

0e2b81f

dalg24 commented Feb 20, 2025

View reviewed changes

libcxx/test/std/atomics/types.pass.cpp Show resolved Hide resolved

dalg24 merged commit affbb4d into llvm:main Feb 21, 2025
78 checks passed

dalg24 deleted the atomic_difference_type branch February 21, 2025 17:55

philnik777 added a commit to philnik777/llvm-project that referenced this pull request Oct 14, 2025

[libc++][C++03] Cherry-pick llvm#123236 and llvm#147835

0394df1

philnik777 added a commit to philnik777/llvm-project that referenced this pull request Oct 14, 2025

[libc++][C++03] Cherry-pick llvm#123236 and llvm#147835

0ec3265

philnik777 added a commit that referenced this pull request Oct 14, 2025

[libc++][C++03] Cherry-pick #123236 and #147835 (#163363)

6fc2318

akadutta pushed a commit to akadutta/llvm-project that referenced this pull request Oct 14, 2025

[libc++][C++03] Cherry-pick llvm#123236 and llvm#147835 (llvm#163363)

fc5f9f5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[libc++] std::atomic primary template should not have a `difference_type` member type #123236

[libc++] std::atomic primary template should not have a `difference_type` member type #123236

Uh oh!

dalg24 commented Jan 16, 2025

Uh oh!

llvmbot commented Jan 16, 2025

Uh oh!

dalg24 Jan 16, 2025

Uh oh!

dalg24 commented Jan 16, 2025

Uh oh!

philnik777 left a comment

Uh oh!

Uh oh!

dalg24 commented Jan 17, 2025 •

edited

Loading

Uh oh!

ldionne commented Jan 20, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[libc++] std::atomic primary template should not have a difference_type member type #123236

[libc++] std::atomic primary template should not have a difference_type member type #123236

Uh oh!

Conversation

dalg24 commented Jan 16, 2025

Uh oh!

llvmbot commented Jan 16, 2025

Uh oh!

dalg24 Jan 16, 2025

Choose a reason for hiding this comment

Uh oh!

dalg24 commented Jan 16, 2025

Uh oh!

philnik777 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dalg24 commented Jan 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldionne commented Jan 20, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[libc++] std::atomic primary template should not have a `difference_type` member type #123236

[libc++] std::atomic primary template should not have a `difference_type` member type #123236

dalg24 commented Jan 17, 2025 •

edited

Loading