[libc++] Use __alloc_traits whenever it is available for consistency

[winner245]

When an allocator-aware container already defines a member type alias __alloc_traits for std::allocator_traits<allocator_type>, we should consistently use __alloc_traits. Mixing the usage of both __alloc_traits and std::allocator_traits can lead to inconsistencies and confusion.

The following are some specific instances from deque with mixed usages of __alloc_traits and allocator_traits<allocator_type>:

llvm-project/libcxx/include/deque

Lines 62 to 64 in 3e62321

	noexcept((__alloc_traits::propagate_on_container_move_assignment::value &&
	is_nothrow_move_assignable<allocator_type>::value) ||
	allocator_traits<allocator_type>::is_always_equal::value);

llvm-project/libcxx/include/deque

Lines 678 to 680 in 3e62321

	(__alloc_traits::propagate_on_container_move_assignment::value &&
	is_nothrow_move_assignable<allocator_type>::value) ||
	allocator_traits<allocator_type>::is_always_equal::value);

llvm-project/libcxx/include/deque

Lines 1383 to 1385 in 3e62321

	(__alloc_traits::propagate_on_container_move_assignment::value &&
	is_nothrow_move_assignable<allocator_type>::value) ||
	allocator_traits<allocator_type>::is_always_equal::value) {

This PR propose to consistently use __alloc_traits.

[github-actions]

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

[frederick-vs-ja]

This is somehow annoying, because the current standard wording seemingly allows these two different allocator_traits's to have inconsistent Traits::is_always_equal::value values. LWG3267 talked about this.

No change requested. I believe we should just go ahead and assume associated Traits::is_always_equal::value values are equal.

[winner245]

Thank you for your careful thought. I agree with you that for containers with a rebound allocator, if XX::is_always_equal::value != YY::is_always_equal::value (where XX and YY represents the original and rebound allocator traits), we would have trouble. But it seems that libc++ implicitly assumes XX::is_always_equal::value == YY::is_always_equal::value (perhaps even XX::pocma::value == YY::pocma::value), which allows us to use __node_traits instead of allocator_traits<allocator_type>. I've also checked the implementations in libstdc++ and MSVC-STL. It seems that they both have this assumption implicitly.

[winner245]

I think if the allocator provided as template argument and the rebound have different propagation properties and behaves differently on propagation, then it is difficult or impossible for implementations to follow the current exception specifications.

I strongly agree with this statement from LWG3267. This also explains why the three major standard implementations all made the implicit assumption that the original and rebound allocator_traits have the same propagation properties.

After further investigation of libc++ implementation of vector<bool>, I observed instances where the original __alloc_traits and rebound __storage_traits are used interchangeably, e.g.,

llvm-project/libcxx/include/__vector/vector_bool.h

Lines 759 to 765 in 43c82a8

	template <class _Allocator>
	inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 vector<bool, _Allocator>&
	vector<bool, _Allocator>::operator=(vector&& __v)
	_NOEXCEPT_(__noexcept_move_assign_container<_Allocator, __alloc_traits>::value) {
	__move_assign(__v, integral_constant<bool, __storage_traits::propagate_on_container_move_assignment::value>());
	return *this;
	}

where the noexcept specification asserts on the original __alloc_traits, while the implementation is based on the rebound __storage_traits. For other cases, it's mainly the rebound __storage_traits used in both the noexcept specification and the implementation, for example,

llvm-project/libcxx/include/__vector/vector_bool.h

Lines 500 to 505 in 43c82a8

	_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 void __move_assign_alloc(vector& __c)
	_NOEXCEPT_(!__storage_traits::propagate_on_container_move_assignment::value ||
	is_nothrow_move_assignable<allocator_type>::value) {
	__move_assign_alloc(
	__c, integral_constant<bool, __storage_traits::propagate_on_container_move_assignment::value>());
	}

I am not sure whether we should make these usages consistent or continue to rely on the implicit assumption while somehow making the assumption more explicit.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Peng Liu (winner245)

Changes

When an allocator-aware container already defines a member type alias __alloc_traits for std::allocator_traits<allocator_type>, we should consistently use __alloc_traits. Mixing the usage of both __alloc_traits and std::allocator_traits can lead to inconsistencies and confusion.

---

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

3 Files Affected:

• (modified) libcxx/include/deque (+4-4)
• (modified) libcxx/include/forward_list (+2-2)
• (modified) libcxx/include/list (+2-2)

diff --git a/libcxx/include/deque b/libcxx/include/deque
index 95200b4801d7ff3..ce29286445d4c45 100644
--- a/libcxx/include/deque
+++ b/libcxx/include/deque
@@ -61,7 +61,7 @@ public:
     deque& operator=(deque&& c)
         noexcept((__alloc_traits::propagate_on_container_move_assignment::value &&
                   is_nothrow_move_assignable<allocator_type>::value) ||
-                 allocator_traits<allocator_type>::is_always_equal::value);
+                 __alloc_traits::is_always_equal::value);
     deque& operator=(initializer_list<value_type> il);
 
     template <class InputIterator>
@@ -133,7 +133,7 @@ public:
     iterator erase(const_iterator p);
     iterator erase(const_iterator f, const_iterator l);
     void swap(deque& c)
-        noexcept(allocator_traits<allocator_type>::is_always_equal::value);  // C++17
+        noexcept(__alloc_traits::is_always_equal::value);  // C++17
     void clear() noexcept;
 };
 
@@ -677,7 +677,7 @@ public:
   _LIBCPP_HIDE_FROM_ABI deque& operator=(deque&& __c) noexcept(
       (__alloc_traits::propagate_on_container_move_assignment::value &&
        is_nothrow_move_assignable<allocator_type>::value) ||
-      allocator_traits<allocator_type>::is_always_equal::value);
+      __alloc_traits::is_always_equal::value);
 
   _LIBCPP_HIDE_FROM_ABI void assign(initializer_list<value_type> __il) { assign(__il.begin(), __il.end()); }
 #  endif // _LIBCPP_CXX03_LANG
@@ -1382,7 +1382,7 @@ template <class _Tp, class _Allocator>
 inline deque<_Tp, _Allocator>& deque<_Tp, _Allocator>::operator=(deque&& __c) noexcept(
     (__alloc_traits::propagate_on_container_move_assignment::value &&
      is_nothrow_move_assignable<allocator_type>::value) ||
-    allocator_traits<allocator_type>::is_always_equal::value) {
+    __alloc_traits::is_always_equal::value) {
   __move_assign(__c, integral_constant<bool, __alloc_traits::propagate_on_container_move_assignment::value>());
   return *this;
 }
diff --git a/libcxx/include/forward_list b/libcxx/include/forward_list
index 4b6ca8ea8587c08..79d144e6421064d 100644
--- a/libcxx/include/forward_list
+++ b/libcxx/include/forward_list
@@ -719,7 +719,7 @@ public:
   _LIBCPP_HIDE_FROM_ABI forward_list& operator=(forward_list&& __x) noexcept(
       (__node_traits::propagate_on_container_move_assignment::value &&
        is_nothrow_move_assignable<allocator_type>::value) ||
-      allocator_traits<allocator_type>::is_always_equal::value);
+      __node_traits::is_always_equal::value);
 
   _LIBCPP_HIDE_FROM_ABI forward_list& operator=(initializer_list<value_type> __il);
 
@@ -1013,7 +1013,7 @@ template <class _Tp, class _Alloc>
 inline forward_list<_Tp, _Alloc>& forward_list<_Tp, _Alloc>::operator=(forward_list&& __x) noexcept(
     (__node_traits::propagate_on_container_move_assignment::value &&
      is_nothrow_move_assignable<allocator_type>::value) ||
-    allocator_traits<allocator_type>::is_always_equal::value) {
+    __node_traits::is_always_equal::value) {
   __move_assign(__x, integral_constant<bool, __node_traits::propagate_on_container_move_assignment::value>());
   return *this;
 }
diff --git a/libcxx/include/list b/libcxx/include/list
index 3fcf796ebc03d04..9cdb7ffd78af02b 100644
--- a/libcxx/include/list
+++ b/libcxx/include/list
@@ -731,7 +731,7 @@ public:
   _LIBCPP_HIDE_FROM_ABI list& operator=(list&& __c) noexcept(
       (__node_alloc_traits::propagate_on_container_move_assignment::value &&
        is_nothrow_move_assignable<__node_allocator>::value) ||
-      allocator_traits<allocator_type>::is_always_equal::value);
+      __node_alloc_traits::is_always_equal::value);
 
   _LIBCPP_HIDE_FROM_ABI list& operator=(initializer_list<value_type> __il) {
     assign(__il.begin(), __il.end());
@@ -1070,7 +1070,7 @@ template <class _Tp, class _Alloc>
 inline list<_Tp, _Alloc>& list<_Tp, _Alloc>::operator=(list&& __c) noexcept(
     (__node_alloc_traits::propagate_on_container_move_assignment::value &&
      is_nothrow_move_assignable<__node_allocator>::value) ||
-    allocator_traits<allocator_type>::is_always_equal::value) {
+    __node_alloc_traits::is_always_equal::value) {
   __move_assign(__c, integral_constant<bool, __node_alloc_traits::propagate_on_container_move_assignment::value>());
   return *this;
 }

[philnik777]

These changes are modifying stuff that was 1:1 standardese I think, so I'm not sure this change leads to any less "confusion".

[winner245]

These changes are modifying stuff that was 1:1 standardese I think, so I'm not sure this change leads to any less "confusion".

For deque, we were mixing __alloc_traits and allocator_traits<allocator_type> in the same code block in several cases, which is a clear inconsistency. Since we've already defined the member type __alloc_traits, I believe consistently using __alloc_traits would reduce confusion.

For list and forward_list, we were mixing __node_alloc_traits (list) or __node_traits (forward_list) with allocator_traits<allocator_type>, which implicitly assumes that rebound allocator traits and allocator_traits<allocator_type> have the same properties. Once LWG3267 is fixed, I believe fixing the inconsistency here would reduce confusion. However, for now I do not want to get into these subtilties due to the implicit assumption made in our implementation. Therefore, I've reverted the changes for list and forward_list.

[philnik777]

Just to be clear, I'm really ambivalent on whether we should make these changes.