[libc++] Remove unnecessary std::vector accessors #114423

ldionne · 2024-10-31T16:10:13Z

Now that we don't use __compressed_pair anymore inside std::vector, we can remove some unnecessary accessors. This is a mechanical replacement of the __alloc() and __end_cap() accessors, and similar for std::vector.

Note that I consistently used this->_alloc instead of just _alloc since most of the code in uses that pattern to access members. I don't think this is necessary anymore (and I'm even less certain I like this), but I went for consistency with surrounding code. If we want to change that, we can do a follow-up mechanical change.

llvmbot · 2024-10-31T16:10:50Z

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

Now that we don't use __compressed_pair anymore inside std::vector, we can remove some unnecessary accessors. This is a mechanical replacement of the __alloc() and __end_cap() accessors, and similar for std::vector<bool>.

Note that I consistently used this->_alloc instead of just _alloc since most of the code in <vector> uses that pattern to access members. I don't think this is necessary anymore (and I'm even less certain I like this), but I went for consistency with surrounding code. If we want to change that, we can do a follow-up mechanical change.

Patch is 33.36 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/114423.diff

2 Files Affected:

(modified) libcxx/include/__vector/vector.h (+71-92)
(modified) libcxx/include/__vector/vector_bool.h (+28-36)

diff --git a/libcxx/include/__vector/vector.h b/libcxx/include/__vector/vector.h
index 844e5d6a210568..ebd894e885c5cc 100644
--- a/libcxx/include/__vector/vector.h
+++ b/libcxx/include/__vector/vector.h
@@ -224,7 +224,7 @@ class _LIBCPP_TEMPLATE_VIS vector {
       if (__vec_.__begin_ != nullptr) {
         __vec_.__clear();
         __vec_.__annotate_delete();
-        __alloc_traits::deallocate(__vec_.__alloc(), __vec_.__begin_, __vec_.capacity());
+        __alloc_traits::deallocate(__vec_.__alloc_, __vec_.__begin_, __vec_.capacity());
       }
     }
 
@@ -298,7 +298,7 @@ class _LIBCPP_TEMPLATE_VIS vector {
 #endif
 
   _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI allocator_type get_allocator() const _NOEXCEPT {
-    return this->__alloc();
+    return this->__alloc_;
   }
 
   _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI iterator begin() _NOEXCEPT;
@@ -330,7 +330,7 @@ class _LIBCPP_TEMPLATE_VIS vector {
     return static_cast<size_type>(this->__end_ - this->__begin_);
   }
   _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI size_type capacity() const _NOEXCEPT {
-    return static_cast<size_type>(__end_cap() - this->__begin_);
+    return static_cast<size_type>(this->__cap_ - this->__begin_);
   }
   [[__nodiscard__]] _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI bool empty() const _NOEXCEPT {
     return this->__begin_ == this->__end_;
@@ -465,17 +465,17 @@ class _LIBCPP_TEMPLATE_VIS vector {
   //  Allocate space for __n objects
   //  throws length_error if __n > max_size()
   //  throws (probably bad_alloc) if memory run out
-  //  Precondition:  __begin_ == __end_ == __end_cap() == 0
+  //  Precondition:  __begin_ == __end_ == __cap_ == nullptr
   //  Precondition:  __n > 0
   //  Postcondition:  capacity() >= __n
   //  Postcondition:  size() == 0
   _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void __vallocate(size_type __n) {
     if (__n > max_size())
       __throw_length_error();
-    auto __allocation = std::__allocate_at_least(__alloc(), __n);
+    auto __allocation = std::__allocate_at_least(this->__alloc_, __n);
     __begin_          = __allocation.ptr;
     __end_            = __allocation.ptr;
-    __end_cap()       = __begin_ + __allocation.count;
+    __cap_            = __begin_ + __allocation.count;
     __annotate_new(0);
   }
 
@@ -544,7 +544,7 @@ class _LIBCPP_TEMPLATE_VIS vector {
     return std::__make_bounded_iter(
         std::__wrap_iter<pointer>(__p),
         std::__wrap_iter<pointer>(this->__begin_),
-        std::__wrap_iter<pointer>(this->__end_cap()));
+        std::__wrap_iter<pointer>(this->__cap_));
 #else
     return iterator(__p);
 #endif // _LIBCPP_ABI_BOUNDED_ITERATORS_IN_VECTOR
@@ -556,7 +556,7 @@ class _LIBCPP_TEMPLATE_VIS vector {
     return std::__make_bounded_iter(
         std::__wrap_iter<const_pointer>(__p),
         std::__wrap_iter<const_pointer>(this->__begin_),
-        std::__wrap_iter<const_pointer>(this->__end_cap()));
+        std::__wrap_iter<const_pointer>(this->__cap_));
 #else
     return const_iterator(__p);
 #endif // _LIBCPP_ABI_BOUNDED_ITERATORS_IN_VECTOR
@@ -649,20 +649,10 @@ class _LIBCPP_TEMPLATE_VIS vector {
   template <class... _Args>
   _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void __construct_one_at_end(_Args&&... __args) {
     _ConstructTransaction __tx(*this, 1);
-    __alloc_traits::construct(this->__alloc(), std::__to_address(__tx.__pos_), std::forward<_Args>(__args)...);
+    __alloc_traits::construct(this->__alloc_, std::__to_address(__tx.__pos_), std::forward<_Args>(__args)...);
     ++__tx.__pos_;
   }
 
-  // TODO: Remove these now redundant accessors
-  _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI allocator_type& __alloc() _NOEXCEPT { return this->__alloc_; }
-  _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI const allocator_type& __alloc() const _NOEXCEPT {
-    return this->__alloc_;
-  }
-  _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI pointer& __end_cap() _NOEXCEPT { return this->__cap_; }
-  _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI const pointer& __end_cap() const _NOEXCEPT {
-    return this->__cap_;
-  }
-
   _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void __clear() _NOEXCEPT {
     __base_destruct_at_end(this->__begin_);
   }
@@ -670,7 +660,7 @@ class _LIBCPP_TEMPLATE_VIS vector {
   _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void __base_destruct_at_end(pointer __new_last) _NOEXCEPT {
     pointer __soon_to_be_end = this->__end_;
     while (__new_last != __soon_to_be_end)
-      __alloc_traits::destroy(__alloc(), std::__to_address(--__soon_to_be_end));
+      __alloc_traits::destroy(this->__alloc_, std::__to_address(--__soon_to_be_end));
     this->__end_ = __new_last;
   }
 
@@ -689,20 +679,20 @@ class _LIBCPP_TEMPLATE_VIS vector {
   [[__noreturn__]] _LIBCPP_HIDE_FROM_ABI void __throw_out_of_range() const { std::__throw_out_of_range("vector"); }
 
   _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void __copy_assign_alloc(const vector& __c, true_type) {
-    if (__alloc() != __c.__alloc()) {
+    if (this->__alloc_ != __c.__alloc_) {
       __clear();
       __annotate_delete();
-      __alloc_traits::deallocate(__alloc(), this->__begin_, capacity());
-      this->__begin_ = this->__end_ = __end_cap() = nullptr;
+      __alloc_traits::deallocate(this->__alloc_, this->__begin_, capacity());
+      this->__begin_ = this->__end_ = this->__cap_ = nullptr;
     }
-    __alloc() = __c.__alloc();
+    this->__alloc_ = __c.__alloc_;
   }
 
   _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void __copy_assign_alloc(const vector&, false_type) {}
 
   _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void __move_assign_alloc(vector& __c, true_type)
       _NOEXCEPT_(is_nothrow_move_assignable<allocator_type>::value) {
-    __alloc() = std::move(__c.__alloc());
+    this->__alloc_ = std::move(__c.__alloc_);
   }
 
   _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void __move_assign_alloc(vector&, false_type) _NOEXCEPT {}
@@ -738,12 +728,12 @@ vector<_Tp, _Allocator>::__swap_out_circular_buffer(__split_buffer<value_type, a
   __annotate_delete();
   auto __new_begin = __v.__begin_ - (__end_ - __begin_);
   std::__uninitialized_allocator_relocate(
-      __alloc(), std::__to_address(__begin_), std::__to_address(__end_), std::__to_address(__new_begin));
+      this->__alloc_, std::__to_address(__begin_), std::__to_address(__end_), std::__to_address(__new_begin));
   __v.__begin_ = __new_begin;
   __end_       = __begin_; // All the objects have been destroyed by relocating them.
   std::swap(this->__begin_, __v.__begin_);
   std::swap(this->__end_, __v.__end_);
-  std::swap(this->__end_cap(), __v.__end_cap());
+  std::swap(this->__cap_, __v.__cap_);
   __v.__first_ = __v.__begin_;
   __annotate_new(size());
 }
@@ -761,19 +751,19 @@ vector<_Tp, _Allocator>::__swap_out_circular_buffer(__split_buffer<value_type, a
   // Relocate [__p, __end_) first to avoid having a hole in [__begin_, __end_)
   // in case something in [__begin_, __p) throws.
   std::__uninitialized_allocator_relocate(
-      __alloc(), std::__to_address(__p), std::__to_address(__end_), std::__to_address(__v.__end_));
+      this->__alloc_, std::__to_address(__p), std::__to_address(__end_), std::__to_address(__v.__end_));
   __v.__end_ += (__end_ - __p);
   __end_           = __p; // The objects in [__p, __end_) have been destroyed by relocating them.
   auto __new_begin = __v.__begin_ - (__p - __begin_);
 
   std::__uninitialized_allocator_relocate(
-      __alloc(), std::__to_address(__begin_), std::__to_address(__p), std::__to_address(__new_begin));
+      this->__alloc_, std::__to_address(__begin_), std::__to_address(__p), std::__to_address(__new_begin));
   __v.__begin_ = __new_begin;
   __end_       = __begin_; // All the objects have been destroyed by relocating them.
 
   std::swap(this->__begin_, __v.__begin_);
   std::swap(this->__end_, __v.__end_);
-  std::swap(this->__end_cap(), __v.__end_cap());
+  std::swap(this->__cap_, __v.__cap_);
   __v.__first_ = __v.__begin_;
   __annotate_new(size());
   return __ret;
@@ -784,15 +774,15 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 void vector<_Tp, _Allocator>::__vdeallocate() _NOE
   if (this->__begin_ != nullptr) {
     clear();
     __annotate_delete();
-    __alloc_traits::deallocate(this->__alloc(), this->__begin_, capacity());
-    this->__begin_ = this->__end_ = this->__end_cap() = nullptr;
+    __alloc_traits::deallocate(this->__alloc_, this->__begin_, capacity());
+    this->__begin_ = this->__end_ = this->__cap_ = nullptr;
   }
 }
 
 template <class _Tp, class _Allocator>
 _LIBCPP_CONSTEXPR_SINCE_CXX20 typename vector<_Tp, _Allocator>::size_type
 vector<_Tp, _Allocator>::max_size() const _NOEXCEPT {
-  return std::min<size_type>(__alloc_traits::max_size(this->__alloc()), numeric_limits<difference_type>::max());
+  return std::min<size_type>(__alloc_traits::max_size(this->__alloc_), numeric_limits<difference_type>::max());
 }
 
 //  Precondition:  __new_size > capacity()
@@ -818,7 +808,7 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 void vector<_Tp, _Allocator>::__construct_at_end(s
   _ConstructTransaction __tx(*this, __n);
   const_pointer __new_end = __tx.__new_end_;
   for (pointer __pos = __tx.__pos_; __pos != __new_end; __tx.__pos_ = ++__pos) {
-    __alloc_traits::construct(this->__alloc(), std::__to_address(__pos));
+    __alloc_traits::construct(this->__alloc_, std::__to_address(__pos));
   }
 }
 
@@ -834,7 +824,7 @@ vector<_Tp, _Allocator>::__construct_at_end(size_type __n, const_reference __x)
   _ConstructTransaction __tx(*this, __n);
   const_pointer __new_end = __tx.__new_end_;
   for (pointer __pos = __tx.__pos_; __pos != __new_end; __tx.__pos_ = ++__pos) {
-    __alloc_traits::construct(this->__alloc(), std::__to_address(__pos), __x);
+    __alloc_traits::construct(this->__alloc_, std::__to_address(__pos), __x);
   }
 }
 
@@ -843,7 +833,7 @@ template <class _InputIterator, class _Sentinel>
 _LIBCPP_CONSTEXPR_SINCE_CXX20 void
 vector<_Tp, _Allocator>::__construct_at_end(_InputIterator __first, _Sentinel __last, size_type __n) {
   _ConstructTransaction __tx(*this, __n);
-  __tx.__pos_ = std::__uninitialized_allocator_copy(__alloc(), __first, __last, __tx.__pos_);
+  __tx.__pos_ = std::__uninitialized_allocator_copy(this->__alloc_, __first, __last, __tx.__pos_);
 }
 
 //  Default constructs __n objects starting at __end_
@@ -852,11 +842,10 @@ vector<_Tp, _Allocator>::__construct_at_end(_InputIterator __first, _Sentinel __
 //  Exception safety: strong.
 template <class _Tp, class _Allocator>
 _LIBCPP_CONSTEXPR_SINCE_CXX20 void vector<_Tp, _Allocator>::__append(size_type __n) {
-  if (static_cast<size_type>(this->__end_cap() - this->__end_) >= __n)
+  if (static_cast<size_type>(this->__cap_ - this->__end_) >= __n)
     this->__construct_at_end(__n);
   else {
-    allocator_type& __a = this->__alloc();
-    __split_buffer<value_type, allocator_type&> __v(__recommend(size() + __n), size(), __a);
+    __split_buffer<value_type, allocator_type&> __v(__recommend(size() + __n), size(), this->__alloc_);
     __v.__construct_at_end(__n);
     __swap_out_circular_buffer(__v);
   }
@@ -868,11 +857,10 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 void vector<_Tp, _Allocator>::__append(size_type _
 //  Exception safety: strong.
 template <class _Tp, class _Allocator>
 _LIBCPP_CONSTEXPR_SINCE_CXX20 void vector<_Tp, _Allocator>::__append(size_type __n, const_reference __x) {
-  if (static_cast<size_type>(this->__end_cap() - this->__end_) >= __n)
+  if (static_cast<size_type>(this->__cap_ - this->__end_) >= __n)
     this->__construct_at_end(__n, __x);
   else {
-    allocator_type& __a = this->__alloc();
-    __split_buffer<value_type, allocator_type&> __v(__recommend(size() + __n), size(), __a);
+    __split_buffer<value_type, allocator_type&> __v(__recommend(size() + __n), size(), this->__alloc_);
     __v.__construct_at_end(__n, __x);
     __swap_out_circular_buffer(__v);
   }
@@ -922,7 +910,7 @@ vector<_Tp, _Allocator>::vector(_ForwardIterator __first, _ForwardIterator __las
 
 template <class _Tp, class _Allocator>
 _LIBCPP_CONSTEXPR_SINCE_CXX20 vector<_Tp, _Allocator>::vector(const vector& __x)
-    : __alloc_(__alloc_traits::select_on_container_copy_construction(__x.__alloc())) {
+    : __alloc_(__alloc_traits::select_on_container_copy_construction(__x.__alloc_)) {
   __init_with_size(__x.__begin_, __x.__end_, __x.size());
 }
 
@@ -940,22 +928,22 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 inline _LIBCPP_HIDE_FROM_ABI vector<_Tp, _Allocato
 #else
     _NOEXCEPT_(is_nothrow_move_constructible<allocator_type>::value)
 #endif
-    : __alloc_(std::move(__x.__alloc())) {
-  this->__begin_    = __x.__begin_;
-  this->__end_      = __x.__end_;
-  this->__end_cap() = __x.__end_cap();
-  __x.__begin_ = __x.__end_ = __x.__end_cap() = nullptr;
+    : __alloc_(std::move(__x.__alloc_)) {
+  this->__begin_ = __x.__begin_;
+  this->__end_   = __x.__end_;
+  this->__cap_   = __x.__cap_;
+  __x.__begin_ = __x.__end_ = __x.__cap_ = nullptr;
 }
 
 template <class _Tp, class _Allocator>
 _LIBCPP_CONSTEXPR_SINCE_CXX20 inline _LIBCPP_HIDE_FROM_ABI
 vector<_Tp, _Allocator>::vector(vector&& __x, const __type_identity_t<allocator_type>& __a)
     : __alloc_(__a) {
-  if (__a == __x.__alloc()) {
-    this->__begin_    = __x.__begin_;
-    this->__end_      = __x.__end_;
-    this->__end_cap() = __x.__end_cap();
-    __x.__begin_ = __x.__end_ = __x.__end_cap() = nullptr;
+  if (__a == __x.__alloc_) {
+    this->__begin_ = __x.__begin_;
+    this->__end_   = __x.__end_;
+    this->__cap_   = __x.__cap_;
+    __x.__begin_ = __x.__end_ = __x.__cap_ = nullptr;
   } else {
     typedef move_iterator<iterator> _Ip;
     auto __guard = std::__make_exception_guard(__destroy_vector(*this));
@@ -992,7 +980,7 @@ vector<_Tp, _Allocator>::operator=(vector&& __x)
 template <class _Tp, class _Allocator>
 _LIBCPP_CONSTEXPR_SINCE_CXX20 void vector<_Tp, _Allocator>::__move_assign(vector& __c, false_type)
     _NOEXCEPT_(__alloc_traits::is_always_equal::value) {
-  if (__alloc() != __c.__alloc()) {
+  if (this->__alloc_ != __c.__alloc_) {
     typedef move_iterator<iterator> _Ip;
     assign(_Ip(__c.begin()), _Ip(__c.end()));
   } else
@@ -1004,10 +992,10 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 void vector<_Tp, _Allocator>::__move_assign(vector
     _NOEXCEPT_(is_nothrow_move_assignable<allocator_type>::value) {
   __vdeallocate();
   __move_assign_alloc(__c); // this can throw
-  this->__begin_    = __c.__begin_;
-  this->__end_      = __c.__end_;
-  this->__end_cap() = __c.__end_cap();
-  __c.__begin_ = __c.__end_ = __c.__end_cap() = nullptr;
+  this->__begin_ = __c.__begin_;
+  this->__end_   = __c.__end_;
+  this->__cap_   = __c.__cap_;
+  __c.__begin_ = __c.__end_ = __c.__cap_ = nullptr;
 }
 
 template <class _Tp, class _Allocator>
@@ -1142,8 +1130,7 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 void vector<_Tp, _Allocator>::reserve(size_type __
   if (__n > capacity()) {
     if (__n > max_size())
       this->__throw_length_error();
-    allocator_type& __a = this->__alloc();
-    __split_buffer<value_type, allocator_type&> __v(__n, size(), __a);
+    __split_buffer<value_type, allocator_type&> __v(__n, size(), this->__alloc_);
     __swap_out_circular_buffer(__v);
   }
 }
@@ -1154,8 +1141,7 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 void vector<_Tp, _Allocator>::shrink_to_fit() _NOE
 #if _LIBCPP_HAS_EXCEPTIONS
     try {
 #endif // _LIBCPP_HAS_EXCEPTIONS
-      allocator_type& __a = this->__alloc();
-      __split_buffer<value_type, allocator_type&> __v(size(), size(), __a);
+      __split_buffer<value_type, allocator_type&> __v(size(), size(), this->__alloc_);
       // The Standard mandates shrink_to_fit() does not increase the capacity.
       // With equal capacity keep the existing buffer. This avoids extra work
       // due to swapping the elements.
@@ -1172,10 +1158,9 @@ template <class _Tp, class _Allocator>
 template <class... _Args>
 _LIBCPP_CONSTEXPR_SINCE_CXX20 typename vector<_Tp, _Allocator>::pointer
 vector<_Tp, _Allocator>::__emplace_back_slow_path(_Args&&... __args) {
-  allocator_type& __a = this->__alloc();
-  __split_buffer<value_type, allocator_type&> __v(__recommend(size() + 1), size(), __a);
+  __split_buffer<value_type, allocator_type&> __v(__recommend(size() + 1), size(), this->__alloc_);
   //    __v.emplace_back(std::forward<_Args>(__args)...);
-  __alloc_traits::construct(__a, std::__to_address(__v.__end_), std::forward<_Args>(__args)...);
+  __alloc_traits::construct(this->__alloc_, std::__to_address(__v.__end_), std::forward<_Args>(__args)...);
   __v.__end_++;
   __swap_out_circular_buffer(__v);
   return this->__end_;
@@ -1191,7 +1176,7 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 inline
 #endif
     vector<_Tp, _Allocator>::emplace_back(_Args&&... __args) {
   pointer __end = this->__end_;
-  if (__end < this->__end_cap()) {
+  if (__end < this->__cap_) {
     __construct_one_at_end(std::forward<_Args>(__args)...);
     ++__end;
   } else {
@@ -1240,7 +1225,7 @@ vector<_Tp, _Allocator>::__move_range(pointer __from_s, pointer __from_e, pointe
     pointer __i = __from_s + __n;
     _ConstructTransaction __tx(*this, __from_e - __i);
     for (pointer __pos = __tx.__pos_; __i < __from_e; ++__i, (void)++__pos, __tx.__pos_ = __pos) {
-      __alloc_traits::construct(this->__alloc(), std::__to_address(__pos), std::move(*__i));
+      __alloc_traits::construct(this->__alloc_, std::__to_address(__pos), std::move(*__i));
     }
   }
   std::move_backward(__from_s, __from_s + __n, __old_last);
@@ -1250,7 +1235,7 @@ template <class _Tp, class _Allocator>
 _LIBCPP_CONSTEXPR_SINCE_CXX20 typename vector<_Tp, _Allocator>::iterator
 vector<_Tp, _Allocator>::insert(const_iterator __position, const_reference __x) {
   pointer __p = this->__begin_ + (__position - begin());
-  if (this->__end_ < this->__end_cap()) {
+  if (this->__end_ < this->__cap_) {
     if (__p == this->__end_) {
       __construct_one_at_end(__x);
     } else {
@@ -1261,8 +1246,7 @@ vector<_Tp, _Allocator>::insert(const_iterator __position, const_reference __x)
       *__p = *__xr;
     }
   } else {
-    allocator_type& __a = this->__alloc();
-    __split_buffer<value_type, allocator_type&> __v(__recommend(size() + 1), __p - this->__begin_, __a);
+    __split_buffer<value_type, allocator_type&> __v(__recommend(size() + 1), __p - this->__begin_, this->__alloc_);
     __v.push_back(__x);
     __p = __swap_out_circular_buffer(__v, __p);
   }
@@ -1273,7 +1257,7 @@ template <class _Tp, class _Allocator>
 _LIBCPP_CONSTEXPR_SINCE_CXX20 typename vector<_Tp, _Allocator>::iterator
 vector<_Tp, _Allocator>::insert(const_iterator __position, value_type&& __x) {
   pointer __p = this->__begin_ + (__position - begin());
-  if (this->__end_ < this->__end_cap()) {
+  if (this->__end_ < this->__cap_) {
     if (__p == this->__end_) {
       __construct_one_at_end(std::move(__x));
     } else {
@@ -1281,8 +1265,7 @@ vector<_Tp, _Allocator>::insert(const_iterator __position, value_type&& __x) {
       *__p = std::move(__x);
     }
   } else {
-    allocator_type& __a = this->__alloc();
-    __split_buffer<value_type, allocator_type&> __v(__recommend(size() + 1), __p - this->__begin_, __a);
+    __split_buffer<value_type, allocator_type&> __v(__recommend(size() + 1), __p - this->__begin_, this->__alloc_);
     __v.push_back(std::move(__x));
     __p = __swap_out_circular_buffer(__v, __p);
   }
@@ -1294,17 +1277,16 @@ template <class... _Args>
 _LIBCPP_CONSTEXPR_SINCE_CXX20 typename vector<_Tp, _Allocator>::iterator
 vector<_Tp, _Allocator>::emplace(const_iterator __position, _Args&&... __args) {
   pointer __p = this->__begin_ + (__position - begin());
-  if (this->__end_ < this->__end_cap()) {
+  if (this->__end_ < this->__cap_) {
     if (__p == this->__end_) {
       __construct_one_at_end(std::forward<_Args>(__args)...);
     } else {
-      __temp_value<value_type, _Allocator> __tmp(this->__alloc(), std::forward<_Args>(__args)...);
+      __temp_value<value_type, _Allocator> __tmp(this->__alloc_, std::forward<_Args>(__args)...);
       __move_range(__p, this->__end_, __p + 1);
       *__p = std::move(__tmp.get());
     }
   } else {
-    allocator_type& __a = this->__alloc();
-    __split_buffer<value_type, allocator_type&> __v(__recommend(size() + 1), __p - this->__begin_, __a);
+    __split_buffer<value_type, allocator_type&> __v(__recommend(size() + 1), __p - this->__begin_, this->__alloc_);
     __v.emplace_back...
[truncated]

winner245 · 2024-10-31T16:22:03Z

I agree with the change. In my recent PRs, I already didn't bother using these accessors.

winner245 · 2024-10-31T16:54:52Z

Maybe we need a more thorough removal as _LIBCPP_COMPRESSED_PAIR has been spread out in many files inlcuding vector.h, deque, forward_list, list, __split_buffer, etc. This also brings in other unnecessary accessors:

__size() and __alloc() for 'deque'
__before_begin() and __alloc() for forward_list
__sz() and __node_alloc() for list
__end_cap() and __alloc() for __split_buffer

This is just a short list in my mind. I am not sure what the best strategy is. But I believe it is worth cleaning up.

ldionne · 2024-11-01T16:41:31Z

Yes, I think we should eventually clean up everything, but I was planning to do it from separate patches to keep it light.

winner245 · 2024-11-01T16:55:25Z

Absolutely, that's a good approach to proceed, as we've already seen so many changes in just vector.h.

…lit_buffer (#115517) Related to PR #114423, this PR proposes to unify the naming of the internal pointer members in `std::vector` and `std::__split_buffer` for consistency and clarity. Both `std::vector` and `std::__split_buffer` originally used a `__compressed_pair<pointer, allocator_type>` member named `__end_cap_` to store an internal capacity pointer and an allocator. However, inconsistent naming changes have been made in both classes: - `std::vector` now uses `__cap_` and `__alloc_` for its internal pointer and allocator members. - In contrast, `std::__split_buffer` retains the name `__end_cap_` for the capacity pointer, along with `__alloc_`. This inconsistency between the names `__cap_` and `__end_cap_` has caused confusions (especially to myself when I was working on both classes). I suggest unifying these names by renaming `__end_cap_` to `__cap_` in `std::__split_buffer`.

Now that we don't use __compressed_pair anymore inside std::vector, we can remove some unnecessary accessors. This is a mechanical replacement of the __alloc() and __end_cap() accessors, and similar for std::vector<bool>. Note that I consistently used this->__alloc_ instead of just __alloc_ since most of the code in <vector> uses that pattern to access members. I don't think this is necessary anymore (and I'm even less certain I like this), but I went for consistency with surrounding code. If we want to change that, we can do a follow-up mechanical change.

ldionne requested a review from a team as a code owner October 31, 2024 16:10

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

winner245 mentioned this pull request Nov 8, 2024

Unify naming of internal pointer members in std::vector and std::__split_buffer #115517

Merged

ldionne force-pushed the review/vector-remove-accessors branch from 6e79055 to 3427ce9 Compare November 11, 2024 17:40

ldionne force-pushed the review/vector-remove-accessors branch 3 times, most recently from 687b447 to 2a21c7a Compare November 14, 2024 23:03

ldionne force-pushed the review/vector-remove-accessors branch from 2a21c7a to cbf9c24 Compare November 17, 2024 22:38

ldionne merged commit 5a48162 into llvm:main Nov 18, 2024
60 of 62 checks passed

ldionne deleted the review/vector-remove-accessors branch November 18, 2024 09:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[libc++] Remove unnecessary std::vector accessors #114423

[libc++] Remove unnecessary std::vector accessors #114423

ldionne commented Oct 31, 2024

llvmbot commented Oct 31, 2024

winner245 commented Oct 31, 2024

winner245 commented Oct 31, 2024

ldionne commented Nov 1, 2024

winner245 commented Nov 1, 2024

[libc++] Remove unnecessary std::vector accessors #114423

[libc++] Remove unnecessary std::vector accessors #114423

Conversation

ldionne commented Oct 31, 2024

llvmbot commented Oct 31, 2024

winner245 commented Oct 31, 2024

winner245 commented Oct 31, 2024

ldionne commented Nov 1, 2024

winner245 commented Nov 1, 2024