Skip to content

[libc++] Fix bug in **tests** for std::atomic_ref<T*> increment and decrement operators #122271

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 10, 2025

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-fix-vnni-instr-types 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