Skip to content

Conversation

@dalg24
Copy link
Member

@dalg24 dalg24 commented Jan 9, 2025

Fix https://github.com/llvm/llvm-project/pull/121414/files#r1908697882

The implementation is fine and has the proper increment/decrement operators defined

_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; }

The issue is in the tests

  • a typo (T instead of std::atomic_ref<T>) when ensuring that increment/decrement operators are not defined in the primary template and specialization for floating point types, and
  • the specialization for pointer types miscategorized.

@dalg24 dalg24 requested a review from a team as a code owner January 9, 2025 13:12
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Jan 9, 2025
@llvmbot
Copy link
Member

llvmbot commented Jan 9, 2025

@llvm/pr-subscribers-libcxx

Author: Damien L-G (dalg24)

Changes

Fix https://github.com/llvm/llvm-project/pull/121414/files#r1908697882

The implementation is fine and has the proper increment/decrement operators defined

_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; }

The issue is in the tests

  • a typo (T instead of std::atomic_ref&lt;T&gt;) when ensuring that increment/decrement operators are not defined in the primary template and specialization for floating point types, and
  • the specialization for pointer types miscategorized.

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

1 Files Affected:

  • (modified) libcxx/test/std/atomics/atomics.ref/increment_decrement.pass.cpp (+69-33)
diff --git a/libcxx/test/std/atomics/atomics.ref/increment_decrement.pass.cpp b/libcxx/test/std/atomics/atomics.ref/increment_decrement.pass.cpp
index f26a0bdf3663a5..0dfd797a908371 100644
--- a/libcxx/test/std/atomics/atomics.ref/increment_decrement.pass.cpp
+++ b/libcxx/test/std/atomics/atomics.ref/increment_decrement.pass.cpp
@@ -42,43 +42,79 @@ constexpr bool does_not_have_increment_nor_decrement_operators() {
 
 template <typename T>
 struct TestDoesNotHaveIncrementDecrement {
-  void operator()() const { static_assert(does_not_have_increment_nor_decrement_operators<T>()); }
+  void operator()() const { static_assert(does_not_have_increment_nor_decrement_operators<std::atomic_ref<T>>()); }
 };
 
 template <typename T>
 struct TestIncrementDecrement {
   void operator()() const {
-    static_assert(std::is_integral_v<T>);
-
-    T x(T(1));
-    std::atomic_ref<T> const a(x);
-
-    {
-      std::same_as<T> decltype(auto) y = ++a;
-      assert(y == T(2));
-      assert(x == T(2));
-      ASSERT_NOEXCEPT(++a);
-    }
-
-    {
-      std::same_as<T> decltype(auto) y = --a;
-      assert(y == T(1));
-      assert(x == T(1));
-      ASSERT_NOEXCEPT(--a);
-    }
-
-    {
-      std::same_as<T> decltype(auto) y = a++;
-      assert(y == T(1));
-      assert(x == T(2));
-      ASSERT_NOEXCEPT(a++);
-    }
-
-    {
-      std::same_as<T> decltype(auto) y = a--;
-      assert(y == T(2));
-      assert(x == T(1));
-      ASSERT_NOEXCEPT(a--);
+    static_assert(std::is_integral_v<T> || std::is_pointer_v<T>);
+    if constexpr (std::is_integral_v<T>) {
+      T x(T(1));
+      std::atomic_ref<T> const a(x);
+
+      {
+        std::same_as<T> decltype(auto) y = ++a;
+        assert(y == T(2));
+        assert(x == T(2));
+        ASSERT_NOEXCEPT(++a);
+      }
+
+      {
+        std::same_as<T> decltype(auto) y = --a;
+        assert(y == T(1));
+        assert(x == T(1));
+        ASSERT_NOEXCEPT(--a);
+      }
+
+      {
+        std::same_as<T> decltype(auto) y = a++;
+        assert(y == T(1));
+        assert(x == T(2));
+        ASSERT_NOEXCEPT(a++);
+      }
+
+      {
+        std::same_as<T> decltype(auto) y = a--;
+        assert(y == T(2));
+        assert(x == T(1));
+        ASSERT_NOEXCEPT(a--);
+      }
+    } else if constexpr (std::is_pointer_v<T>) {
+      using U = std::remove_pointer_t<T>;
+      U t[9]  = {};
+      T p{&t[1]};
+      std::atomic_ref<T> const a(p);
+
+      {
+        std::same_as<T> decltype(auto) y = ++a;
+        assert(y == &t[2]);
+        assert(p == &t[2]);
+        ASSERT_NOEXCEPT(++a);
+      }
+
+      {
+        std::same_as<T> decltype(auto) y = --a;
+        assert(y == &t[1]);
+        assert(p == &t[1]);
+        ASSERT_NOEXCEPT(--a);
+      }
+
+      {
+        std::same_as<T> decltype(auto) y = a++;
+        assert(y == &t[1]);
+        assert(p == &t[2]);
+        ASSERT_NOEXCEPT(a++);
+      }
+
+      {
+        std::same_as<T> decltype(auto) y = a--;
+        assert(y == &t[2]);
+        assert(p == &t[1]);
+        ASSERT_NOEXCEPT(a--);
+      }
+    } else {
+      static_assert(std::is_void_v<T>);
     }
   }
 };
@@ -88,7 +124,7 @@ int main(int, char**) {
 
   TestEachFloatingPointType<TestDoesNotHaveIncrementDecrement>()();
 
-  TestEachPointerType<TestDoesNotHaveIncrementDecrement>()();
+  TestEachPointerType<TestIncrementDecrement>()();
 
   TestDoesNotHaveIncrementDecrement<bool>()();
   TestDoesNotHaveIncrementDecrement<UserAtomicType>()();

@ldionne ldionne merged commit 19557a4 into llvm:main Jan 10, 2025
62 checks passed
@dalg24 dalg24 deleted the atomic_ref_fix_increment_decrement_test branch January 10, 2025 21:43
BaiXilin pushed a commit to BaiXilin/llvm-project that referenced this pull request Jan 12, 2025
…ment operators (llvm#122271)

The implementation is fine and has the proper increment/decrement
operators defined, but the tests were wrong:
- a typo (`T` instead of `std::atomic_ref<T>`) when ensuring that increment/decrement
  operators are not defined in the primary template and specialization for floating point
  types, and
- the specialization for pointer types was miscategorized.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants