[libc++] Move allocator assertion into allocator_traits #94750

huixie90 · 2024-06-07T13:04:38Z

There is code duplication in all containers that static_assert the allocator matches the allocator requirements in the spec. This check can be moved into a more centralised place.

llvmbot · 2024-06-07T13:05:12Z

@llvm/pr-subscribers-libcxx

Author: Hui (huixie90)

Changes

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

12 Files Affected:

(modified) libcxx/include/__memory/allocator_traits.h (+11)
(modified) libcxx/include/deque (-3)
(modified) libcxx/include/forward_list (-4)
(modified) libcxx/include/list (-4)
(modified) libcxx/include/map (-8)
(modified) libcxx/include/set (-8)
(modified) libcxx/include/string (-4)
(modified) libcxx/include/unordered_map (-8)
(modified) libcxx/include/unordered_set (-4)
(modified) libcxx/include/vector (-4)
(modified) libcxx/test/std/utilities/memory/allocator.traits/allocator.traits.types/rebind_alloc.pass.cpp (+6-6)
(modified) libcxx/test/std/utilities/memory/allocator.traits/rebind_traits.pass.cpp (+6-6)

diff --git a/libcxx/include/__memory/allocator_traits.h b/libcxx/include/__memory/allocator_traits.h
index 47fe132d15cb1..515306de1ea0c 100644
--- a/libcxx/include/__memory/allocator_traits.h
+++ b/libcxx/include/__memory/allocator_traits.h
@@ -362,6 +362,17 @@ struct _LIBCPP_TEMPLATE_VIS allocator_traits {
   select_on_container_copy_construction(const allocator_type& __a) {
     return __a;
   }
+
+private:
+#ifndef _LIBCPP_CXX03_LANG
+  using __rebind_self = rebind_alloc<value_type>;
+#else
+  using __rebind_self = typename rebind_alloc<value_type>::other;
+#endif
+
+  static_assert(is_same<allocator_type, __rebind_self>::value,
+                "[allocator.requirements] states that rebinding an allocator to the same type should result in the "
+                "original allocator");
 };
 
 #ifndef _LIBCPP_CXX03_LANG
diff --git a/libcxx/include/deque b/libcxx/include/deque
index 555761aae6afd..0e7fac85b2a02 100644
--- a/libcxx/include/deque
+++ b/libcxx/include/deque
@@ -477,9 +477,6 @@ public:
   using reverse_iterator       = std::reverse_iterator<iterator>;
   using const_reverse_iterator = std::reverse_iterator<const_iterator>;
 
-  static_assert(is_same<allocator_type, __rebind_alloc<__alloc_traits, value_type> >::value,
-                "[allocator.requirements] states that rebinding an allocator to the same type should result in the "
-                "original allocator");
   static_assert(is_nothrow_default_constructible<allocator_type>::value ==
                     is_nothrow_default_constructible<__pointer_allocator>::value,
                 "rebinding an allocator should not change excpetion guarantees");
diff --git a/libcxx/include/forward_list b/libcxx/include/forward_list
index 363931e3f2388..9fa7a5737683b 100644
--- a/libcxx/include/forward_list
+++ b/libcxx/include/forward_list
@@ -653,10 +653,6 @@ public:
   static_assert(is_same<value_type, typename allocator_type::value_type>::value,
                 "Allocator::value_type must be same type as value_type");
 
-  static_assert(is_same<allocator_type, __rebind_alloc<allocator_traits<allocator_type>, value_type> >::value,
-                "[allocator.requirements] states that rebinding an allocator to the same type should result in the "
-                "original allocator");
-
   static_assert((!is_same<allocator_type, __node_allocator>::value),
                 "internal allocator type must differ from user-specified "
                 "type; otherwise overload resolution breaks");
diff --git a/libcxx/include/list b/libcxx/include/list
index 87f15e144ac8f..36098165f50cd 100644
--- a/libcxx/include/list
+++ b/libcxx/include/list
@@ -692,10 +692,6 @@ public:
   typedef void __remove_return_type;
 #endif
 
-  static_assert(is_same<allocator_type, __rebind_alloc<allocator_traits<allocator_type>, value_type> >::value,
-                "[allocator.requirements] states that rebinding an allocator to the same type should result in the "
-                "original allocator");
-
   _LIBCPP_HIDE_FROM_ABI list() _NOEXCEPT_(is_nothrow_default_constructible<__node_allocator>::value) {}
   _LIBCPP_HIDE_FROM_ABI explicit list(const allocator_type& __a) : base(__a) {}
   _LIBCPP_HIDE_FROM_ABI explicit list(size_type __n);
diff --git a/libcxx/include/map b/libcxx/include/map
index 7efa715e84aa7..808c966279063 100644
--- a/libcxx/include/map
+++ b/libcxx/include/map
@@ -999,10 +999,6 @@ private:
   typedef typename __base::__node_traits __node_traits;
   typedef allocator_traits<allocator_type> __alloc_traits;
 
-  static_assert(is_same<allocator_type, __rebind_alloc<__alloc_traits, value_type> >::value,
-                "[allocator.requirements] states that rebinding an allocator to the same type should result in the "
-                "original allocator");
-
   __base __tree_;
 
 public:
@@ -1684,10 +1680,6 @@ private:
   typedef typename __base::__node_traits __node_traits;
   typedef allocator_traits<allocator_type> __alloc_traits;
 
-  static_assert(is_same<allocator_type, __rebind_alloc<__alloc_traits, value_type> >::value,
-                "[allocator.requirements] states that rebinding an allocator to the same type should result in the "
-                "original allocator");
-
   __base __tree_;
 
 public:
diff --git a/libcxx/include/set b/libcxx/include/set
index ab3a4363499af..9d7d894f18ce1 100644
--- a/libcxx/include/set
+++ b/libcxx/include/set
@@ -578,10 +578,6 @@ private:
   typedef __tree<value_type, value_compare, allocator_type> __base;
   typedef allocator_traits<allocator_type> __alloc_traits;
 
-  static_assert(is_same<allocator_type, __rebind_alloc<__alloc_traits, value_type> >::value,
-                "[allocator.requirements] states that rebinding an allocator to the same type should result in the "
-                "original allocator");
-
   __base __tree_;
 
 public:
@@ -1035,10 +1031,6 @@ private:
   typedef __tree<value_type, value_compare, allocator_type> __base;
   typedef allocator_traits<allocator_type> __alloc_traits;
 
-  static_assert(is_same<allocator_type, __rebind_alloc<__alloc_traits, value_type> >::value,
-                "[allocator.requirements] states that rebinding an allocator to the same type should result in the "
-                "original allocator");
-
   __base __tree_;
 
 public:
diff --git a/libcxx/include/string b/libcxx/include/string
index 1db803e822d72..c822c43bc06d6 100644
--- a/libcxx/include/string
+++ b/libcxx/include/string
@@ -782,10 +782,6 @@ public:
   static_assert((is_same<typename allocator_type::value_type, value_type>::value),
                 "Allocator::value_type must be same type as value_type");
 
-  static_assert(is_same<allocator_type, __rebind_alloc<__alloc_traits, value_type> >::value,
-                "[allocator.requirements] states that rebinding an allocator to the same type should result in the "
-                "original allocator");
-
   // TODO: Implement iterator bounds checking without requiring the global database.
   typedef __wrap_iter<pointer> iterator;
   typedef __wrap_iter<const_pointer> const_iterator;
diff --git a/libcxx/include/unordered_map b/libcxx/include/unordered_map
index 2e25b0f050695..4c64659efda25 100644
--- a/libcxx/include/unordered_map
+++ b/libcxx/include/unordered_map
@@ -1059,10 +1059,6 @@ private:
   typedef unique_ptr<__node, _Dp> __node_holder;
   typedef allocator_traits<allocator_type> __alloc_traits;
 
-  static_assert(is_same<allocator_type, __rebind_alloc<__alloc_traits, value_type> >::value,
-                "[allocator.requirements] states that rebinding an allocator to the same type should result in the "
-                "original allocator");
-
   static_assert((is_same<typename __table::__container_value_type, value_type>::value), "");
   static_assert((is_same<typename __table::__node_value_type, __value_type>::value), "");
 
@@ -1866,10 +1862,6 @@ private:
   static_assert((is_same<typename __node_traits::size_type, typename __alloc_traits::size_type>::value),
                 "Allocator uses different size_type for different types");
 
-  static_assert(is_same<allocator_type, __rebind_alloc<__alloc_traits, value_type> >::value,
-                "[allocator.requirements] states that rebinding an allocator to the same type should result in the "
-                "original allocator");
-
 public:
   typedef typename __alloc_traits::pointer pointer;
   typedef typename __alloc_traits::const_pointer const_pointer;
diff --git a/libcxx/include/unordered_set b/libcxx/include/unordered_set
index c966cc8eb4df1..056b1017253a6 100644
--- a/libcxx/include/unordered_set
+++ b/libcxx/include/unordered_set
@@ -591,10 +591,6 @@ public:
   static_assert((is_same<value_type, typename allocator_type::value_type>::value),
                 "Allocator::value_type must be same type as value_type");
 
-  static_assert(is_same<allocator_type, __rebind_alloc<allocator_traits<allocator_type>, value_type> >::value,
-                "[allocator.requirements] states that rebinding an allocator to the same type should result in the "
-                "original allocator");
-
 private:
   typedef __hash_table<value_type, hasher, key_equal, allocator_type> __table;
 
diff --git a/libcxx/include/vector b/libcxx/include/vector
index cbfc2cefa1fd9..52fd41ea0c273 100644
--- a/libcxx/include/vector
+++ b/libcxx/include/vector
@@ -409,10 +409,6 @@ public:
   static_assert((is_same<typename allocator_type::value_type, value_type>::value),
                 "Allocator::value_type must be same type as value_type");
 
-  static_assert(is_same<allocator_type, __rebind_alloc<__alloc_traits, value_type> >::value,
-                "[allocator.requirements] states that rebinding an allocator to the same type should result in the "
-                "original allocator");
-
   _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI vector()
       _NOEXCEPT_(is_nothrow_default_constructible<allocator_type>::value) {}
   _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI explicit vector(const allocator_type& __a)
diff --git a/libcxx/test/std/utilities/memory/allocator.traits/allocator.traits.types/rebind_alloc.pass.cpp b/libcxx/test/std/utilities/memory/allocator.traits/allocator.traits.types/rebind_alloc.pass.cpp
index d0d8476a89a9e..e461e8811b9f8 100644
--- a/libcxx/test/std/utilities/memory/allocator.traits/allocator.traits.types/rebind_alloc.pass.cpp
+++ b/libcxx/test/std/utilities/memory/allocator.traits/allocator.traits.types/rebind_alloc.pass.cpp
@@ -28,7 +28,7 @@ struct A
 {
     typedef T value_type;
 
-    template <class U> struct rebind {typedef ReboundA<U> other;};
+    template <class U> struct rebind {typedef A<U> other;};
 };
 
 template <class T, class U>
@@ -39,7 +39,7 @@ struct B
 {
     typedef T value_type;
 
-    template <class V> struct rebind {typedef ReboundB<V, U> other;};
+    template <class V> struct rebind {typedef B<V, U> other;};
 };
 
 template <class T>
@@ -83,16 +83,16 @@ struct G {
 int main(int, char**)
 {
 #if TEST_STD_VER >= 11
-    static_assert((std::is_same<std::allocator_traits<A<char> >::rebind_alloc<double>, ReboundA<double> >::value), "");
-    static_assert((std::is_same<std::allocator_traits<B<int, char> >::rebind_alloc<double>, ReboundB<double, char> >::value), "");
+    static_assert((std::is_same<std::allocator_traits<A<char> >::rebind_alloc<double>, A<double> >::value), "");
+    static_assert((std::is_same<std::allocator_traits<B<int, char> >::rebind_alloc<double>, B<double, char> >::value), "");
     static_assert((std::is_same<std::allocator_traits<C<char> >::rebind_alloc<double>, C<double> >::value), "");
     static_assert((std::is_same<std::allocator_traits<D<int, char> >::rebind_alloc<double>, D<double, char> >::value), "");
     static_assert((std::is_same<std::allocator_traits<E<char> >::rebind_alloc<double>, E<double> >::value), "");
     static_assert((std::is_same<std::allocator_traits<F<char> >::rebind_alloc<double>, F<double> >::value), "");
     static_assert((std::is_same<std::allocator_traits<G<char> >::rebind_alloc<double>, G<double> >::value), "");
 #else
-    static_assert((std::is_same<std::allocator_traits<A<char> >::rebind_alloc<double>::other, ReboundA<double> >::value), "");
-    static_assert((std::is_same<std::allocator_traits<B<int, char> >::rebind_alloc<double>::other, ReboundB<double, char> >::value), "");
+    static_assert((std::is_same<std::allocator_traits<A<char> >::rebind_alloc<double>::other, A<double> >::value), "");
+    static_assert((std::is_same<std::allocator_traits<B<int, char> >::rebind_alloc<double>::other, B<double, char> >::value), "");
     static_assert((std::is_same<std::allocator_traits<C<char> >::rebind_alloc<double>::other, C<double> >::value), "");
     static_assert((std::is_same<std::allocator_traits<D<int, char> >::rebind_alloc<double>::other, D<double, char> >::value), "");
     static_assert((std::is_same<std::allocator_traits<E<char> >::rebind_alloc<double>::other, E<double> >::value), "");
diff --git a/libcxx/test/std/utilities/memory/allocator.traits/rebind_traits.pass.cpp b/libcxx/test/std/utilities/memory/allocator.traits/rebind_traits.pass.cpp
index 01aac9445c838..ff0abbf5a4f57 100644
--- a/libcxx/test/std/utilities/memory/allocator.traits/rebind_traits.pass.cpp
+++ b/libcxx/test/std/utilities/memory/allocator.traits/rebind_traits.pass.cpp
@@ -28,7 +28,7 @@ struct A
 {
     typedef T value_type;
 
-    template <class U> struct rebind {typedef ReboundA<U> other;};
+    template <class U> struct rebind {typedef A<U> other;};
 };
 
 template <class T, class U>
@@ -39,7 +39,7 @@ struct B
 {
     typedef T value_type;
 
-    template <class V> struct rebind {typedef ReboundB<V, U> other;};
+    template <class V> struct rebind {typedef B<V, U> other;};
 };
 
 template <class T>
@@ -65,14 +65,14 @@ struct E
 int main(int, char**)
 {
 #if TEST_STD_VER >= 11
-    static_assert((std::is_same<std::allocator_traits<A<char> >::rebind_traits<double>, std::allocator_traits<ReboundA<double> > >::value), "");
-    static_assert((std::is_same<std::allocator_traits<B<int, char> >::rebind_traits<double>, std::allocator_traits<ReboundB<double, char> > >::value), "");
+    static_assert((std::is_same<std::allocator_traits<A<char> >::rebind_traits<double>, std::allocator_traits<A<double> > >::value), "");
+    static_assert((std::is_same<std::allocator_traits<B<int, char> >::rebind_traits<double>, std::allocator_traits<B<double, char> > >::value), "");
     static_assert((std::is_same<std::allocator_traits<C<char> >::rebind_traits<double>, std::allocator_traits<C<double> > >::value), "");
     static_assert((std::is_same<std::allocator_traits<D<int, char> >::rebind_traits<double>, std::allocator_traits<D<double, char> > >::value), "");
     static_assert((std::is_same<std::allocator_traits<E<char> >::rebind_traits<double>, std::allocator_traits<E<double> > >::value), "");
 #else
-    static_assert((std::is_same<std::allocator_traits<A<char> >::rebind_traits<double>::other, std::allocator_traits<ReboundA<double> > >::value), "");
-    static_assert((std::is_same<std::allocator_traits<B<int, char> >::rebind_traits<double>::other, std::allocator_traits<ReboundB<double, char> > >::value), "");
+    static_assert((std::is_same<std::allocator_traits<A<char> >::rebind_traits<double>::other, std::allocator_traits<A<double> > >::value), "");
+    static_assert((std::is_same<std::allocator_traits<B<int, char> >::rebind_traits<double>::other, std::allocator_traits<B<double, char> > >::value), "");
     static_assert((std::is_same<std::allocator_traits<C<char> >::rebind_traits<double>::other, std::allocator_traits<C<double> > >::value), "");
     static_assert((std::is_same<std::allocator_traits<D<int, char> >::rebind_traits<double>::other, std::allocator_traits<D<double, char> > >::value), "");
     static_assert((std::is_same<std::allocator_traits<E<char> >::rebind_traits<double>::other, std::allocator_traits<E<double> > >::value), "");

github-actions · 2024-06-07T13:08:39Z

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

ldionne

One caveat with this approach is that if a user specializes allocator_traits, they don't get the diagnostic anymore.

ldionne

LGTM with nitpicks.

libcxx/include/__memory/allocator_traits.h

libcxx/include/deque

libcxx/include/list

EricWF · 2024-06-07T14:04:51Z

libcxx/test/std/utilities/memory/allocator.traits/allocator.traits.types/rebind_alloc.pass.cpp

@@ -28,7 +28,7 @@ struct A
 {
    typedef T value_type;

-    template <class U> struct rebind {typedef ReboundA<U> other;};
+    template <class U> struct rebind {typedef A<U> other;};


I think the point of this test is to check that the rebind template is actually used, vs having the generic implementation apply the rebinding.

The top test no longer establishes that as far as I can tell.
The bottom may still do that, but it's less clear.

Anyway this test has been reverted . but arguably the test is incorrect. as stated in all the static_assert in our containers. rebindAlloc::value_type::type should be the same as Alloc and these tests are not correct.

There is code duplication in all containers that static_assert the allocator matches the allocator requirements in the spec. This check can be moved into a more centralised place.

huixie90 requested a review from a team as a code owner June 7, 2024 13:04

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

ldionne reviewed Jun 7, 2024

View reviewed changes

huixie90 force-pushed the hxie/allocator_assert branch 3 times, most recently from e886812 to f81de45 Compare June 10, 2024 06:56

ldionne approved these changes Jun 14, 2024

View reviewed changes

libcxx/include/__memory/allocator_traits.h Outdated Show resolved Hide resolved

libcxx/include/deque Outdated Show resolved Hide resolved

libcxx/include/list Outdated Show resolved Hide resolved

huixie90 force-pushed the hxie/allocator_assert branch from f81de45 to 9503536 Compare June 17, 2024 18:33

[libc++] reuse allocator validitity check logic and message

76eb60f

huixie90 force-pushed the hxie/allocator_assert branch from 9503536 to 76eb60f Compare June 19, 2024 20:04

EricWF reviewed Jun 19, 2024

View reviewed changes

ldionne merged commit 79e8a59 into llvm:main Jun 25, 2024
56 of 57 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[libc++] Move allocator assertion into allocator_traits #94750

[libc++] Move allocator assertion into allocator_traits #94750

Uh oh!

huixie90 commented Jun 7, 2024 •

edited

Loading

Uh oh!

llvmbot commented Jun 7, 2024

Uh oh!

github-actions bot commented Jun 7, 2024 •

edited

Loading

Uh oh!

ldionne left a comment

Uh oh!

ldionne left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

EricWF Jun 7, 2024

Uh oh!

huixie90 Jun 22, 2024

Uh oh!

Uh oh!

Uh oh!

[libc++] Move allocator assertion into allocator_traits #94750

[libc++] Move allocator assertion into allocator_traits #94750

Uh oh!

Conversation

huixie90 commented Jun 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jun 7, 2024

Uh oh!

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

Uh oh!

Uh oh!

ldionne left a comment

Choose a reason for hiding this comment

Uh oh!

ldionne left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

EricWF Jun 7, 2024

Choose a reason for hiding this comment

Uh oh!

huixie90 Jun 22, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

huixie90 commented Jun 7, 2024 •

edited

Loading

github-actions bot commented Jun 7, 2024 •

edited

Loading