Skip to content

Conversation

winner245
Copy link
Contributor

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.

@winner245 winner245 requested a review from a team as a code owner November 8, 2024 16:58
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Nov 8, 2024
@llvmbot
Copy link
Member

llvmbot commented Nov 8, 2024

@llvm/pr-subscribers-lldb

@llvm/pr-subscribers-libcxx

Author: Peng Liu (winner245)

Changes

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&lt;pointer, allocator_type&gt; 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.


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

3 Files Affected:

  • (modified) libcxx/include/__split_buffer (+31-31)
  • (modified) libcxx/include/__vector/vector.h (+2-2)
  • (modified) libcxx/include/deque (+4-4)
diff --git a/libcxx/include/__split_buffer b/libcxx/include/__split_buffer
index 2a2f2625c748b2..419eee23882547 100644
--- a/libcxx/include/__split_buffer
+++ b/libcxx/include/__split_buffer
@@ -47,7 +47,7 @@ _LIBCPP_PUSH_MACROS
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 // __split_buffer allocates a contiguous chunk of memory and stores objects in the range [__begin_, __end_).
-// It has uninitialized memory in the ranges  [__first_, __begin_) and [__end_, __end_cap_.first()). That allows
+// It has uninitialized memory in the ranges  [__first_, __begin_) and [__end_, __cap_). That allows
 // it to grow both in the front and back without having to move the data.
 
 template <class _Tp, class _Allocator = allocator<_Tp> >
@@ -78,20 +78,20 @@ public:
   pointer __first_;
   pointer __begin_;
   pointer __end_;
-  _LIBCPP_COMPRESSED_PAIR(pointer, __end_cap_, allocator_type, __alloc_);
+  _LIBCPP_COMPRESSED_PAIR(pointer, __cap_, allocator_type, __alloc_);
 
   __split_buffer(const __split_buffer&)            = delete;
   __split_buffer& operator=(const __split_buffer&) = delete;
 
   _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI __split_buffer()
       _NOEXCEPT_(is_nothrow_default_constructible<allocator_type>::value)
-      : __first_(nullptr), __begin_(nullptr), __end_(nullptr), __end_cap_(nullptr) {}
+      : __first_(nullptr), __begin_(nullptr), __end_(nullptr), __cap_(nullptr) {}
 
   _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI explicit __split_buffer(__alloc_rr& __a)
-      : __first_(nullptr), __begin_(nullptr), __end_(nullptr), __end_cap_(nullptr), __alloc_(__a) {}
+      : __first_(nullptr), __begin_(nullptr), __end_(nullptr), __cap_(nullptr), __alloc_(__a) {}
 
   _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI explicit __split_buffer(const __alloc_rr& __a)
-      : __first_(nullptr), __begin_(nullptr), __end_(nullptr), __end_cap_(nullptr), __alloc_(__a) {}
+      : __first_(nullptr), __begin_(nullptr), __end_(nullptr), __cap_(nullptr), __alloc_(__a) {}
 
   _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI
   __split_buffer(size_type __cap, size_type __start, __alloc_rr& __a);
@@ -123,7 +123,7 @@ public:
   _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI bool empty() const { return __end_ == __begin_; }
 
   _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI size_type capacity() const {
-    return static_cast<size_type>(__end_cap_ - __first_);
+    return static_cast<size_type>(__cap_ - __first_);
   }
 
   _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI size_type __front_spare() const {
@@ -131,7 +131,7 @@ public:
   }
 
   _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI size_type __back_spare() const {
-    return static_cast<size_type>(__end_cap_ - __end_);
+    return static_cast<size_type>(__cap_ - __end_);
   }
 
   _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI reference front() { return *__begin_; }
@@ -219,14 +219,14 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 bool __split_buffer<_Tp, _Allocator>::__invariants
       return false;
     if (__end_ != nullptr)
       return false;
-    if (__end_cap_ != nullptr)
+    if (__cap_ != nullptr)
       return false;
   } else {
     if (__begin_ < __first_)
       return false;
     if (__end_ < __begin_)
       return false;
-    if (__end_cap_ < __end_)
+    if (__cap_ < __end_)
       return false;
   }
   return true;
@@ -273,8 +273,8 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 void
 __split_buffer<_Tp, _Allocator>::__construct_at_end_with_sentinel(_Iterator __first, _Sentinel __last) {
   __alloc_rr& __a = __alloc_;
   for (; __first != __last; ++__first) {
-    if (__end_ == __end_cap_) {
-      size_type __old_cap = __end_cap_ - __first_;
+    if (__end_ == __cap_) {
+      size_type __old_cap = __cap_ - __first_;
       size_type __new_cap = std::max<size_type>(2 * __old_cap, 8);
       __split_buffer __buf(__new_cap, 0, __a);
       for (pointer __p = __begin_; __p != __end_; ++__p, (void)++__buf.__end_)
@@ -331,7 +331,7 @@ __split_buffer<_Tp, _Allocator>::__destruct_at_end(pointer __new_last, true_type
 template <class _Tp, class _Allocator>
 _LIBCPP_CONSTEXPR_SINCE_CXX20
 __split_buffer<_Tp, _Allocator>::__split_buffer(size_type __cap, size_type __start, __alloc_rr& __a)
-    : __end_cap_(nullptr), __alloc_(__a) {
+    : __cap_(nullptr), __alloc_(__a) {
   if (__cap == 0) {
     __first_ = nullptr;
   } else {
@@ -340,7 +340,7 @@ __split_buffer<_Tp, _Allocator>::__split_buffer(size_type __cap, size_type __sta
     __cap             = __allocation.count;
   }
   __begin_ = __end_ = __first_ + __start;
-  __end_cap_        = __first_ + __cap;
+  __cap_        = __first_ + __cap;
 }
 
 template <class _Tp, class _Allocator>
@@ -356,32 +356,32 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 __split_buffer<_Tp, _Allocator>::__split_buffer(__
     : __first_(std::move(__c.__first_)),
       __begin_(std::move(__c.__begin_)),
       __end_(std::move(__c.__end_)),
-      __end_cap_(std::move(__c.__end_cap_)),
+      __cap_(std::move(__c.__cap_)),
       __alloc_(std::move(__c.__alloc_)) {
   __c.__first_   = nullptr;
   __c.__begin_   = nullptr;
   __c.__end_     = nullptr;
-  __c.__end_cap_ = nullptr;
+  __c.__cap_ = nullptr;
 }
 
 template <class _Tp, class _Allocator>
 _LIBCPP_CONSTEXPR_SINCE_CXX20
 __split_buffer<_Tp, _Allocator>::__split_buffer(__split_buffer&& __c, const __alloc_rr& __a)
-    : __end_cap_(nullptr), __alloc_(__a) {
+    : __cap_(nullptr), __alloc_(__a) {
   if (__a == __c.__alloc_) {
     __first_       = __c.__first_;
     __begin_       = __c.__begin_;
     __end_         = __c.__end_;
-    __end_cap_     = __c.__end_cap_;
+    __cap_     = __c.__cap_;
     __c.__first_   = nullptr;
     __c.__begin_   = nullptr;
     __c.__end_     = nullptr;
-    __c.__end_cap_ = nullptr;
+    __c.__cap_ = nullptr;
   } else {
     auto __allocation = std::__allocate_at_least(__alloc_, __c.size());
     __first_          = __allocation.ptr;
     __begin_ = __end_ = __first_;
-    __end_cap_        = __first_ + __allocation.count;
+    __cap_        = __first_ + __allocation.count;
     typedef move_iterator<iterator> _Ip;
     __construct_at_end(_Ip(__c.begin()), _Ip(__c.end()));
   }
@@ -398,9 +398,9 @@ __split_buffer<_Tp, _Allocator>::operator=(__split_buffer&& __c)
   __first_   = __c.__first_;
   __begin_   = __c.__begin_;
   __end_     = __c.__end_;
-  __end_cap_ = __c.__end_cap_;
+  __cap_ = __c.__cap_;
   __move_assign_alloc(__c, integral_constant<bool, __alloc_traits::propagate_on_container_move_assignment::value>());
-  __c.__first_ = __c.__begin_ = __c.__end_ = __c.__end_cap_ = nullptr;
+  __c.__first_ = __c.__begin_ = __c.__end_ = __c.__cap_ = nullptr;
   return *this;
 }
 
@@ -410,7 +410,7 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 void __split_buffer<_Tp, _Allocator>::swap(__split
   std::swap(__first_, __x.__first_);
   std::swap(__begin_, __x.__begin_);
   std::swap(__end_, __x.__end_);
-  std::swap(__end_cap_, __x.__end_cap_);
+  std::swap(__cap_, __x.__cap_);
   std::__swap_allocator(__alloc_, __x.__alloc_);
 }
 
@@ -422,7 +422,7 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 void __split_buffer<_Tp, _Allocator>::reserve(size
     std::swap(__first_, __t.__first_);
     std::swap(__begin_, __t.__begin_);
     std::swap(__end_, __t.__end_);
-    std::swap(__end_cap_, __t.__end_cap_);
+    std::swap(__cap_, __t.__cap_);
   }
 }
 
@@ -438,7 +438,7 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 void __split_buffer<_Tp, _Allocator>::shrink_to_fi
       std::swap(__first_, __t.__first_);
       std::swap(__begin_, __t.__begin_);
       std::swap(__end_, __t.__end_);
-      std::swap(__end_cap_, __t.__end_cap_);
+      std::swap(__cap_, __t.__cap_);
 #if _LIBCPP_HAS_EXCEPTIONS
     } catch (...) {
     }
@@ -450,19 +450,19 @@ template <class _Tp, class _Allocator>
 template <class... _Args>
 _LIBCPP_CONSTEXPR_SINCE_CXX20 void __split_buffer<_Tp, _Allocator>::emplace_front(_Args&&... __args) {
   if (__begin_ == __first_) {
-    if (__end_ < __end_cap_) {
-      difference_type __d = __end_cap_ - __end_;
+    if (__end_ < __cap_) {
+      difference_type __d = __cap_ - __end_;
       __d                 = (__d + 1) / 2;
       __begin_            = std::move_backward(__begin_, __end_, __end_ + __d);
       __end_ += __d;
     } else {
-      size_type __c = std::max<size_type>(2 * static_cast<size_t>(__end_cap_ - __first_), 1);
+      size_type __c = std::max<size_type>(2 * static_cast<size_t>(__cap_ - __first_), 1);
       __split_buffer<value_type, __alloc_rr&> __t(__c, (__c + 3) / 4, __alloc_);
       __t.__construct_at_end(move_iterator<pointer>(__begin_), move_iterator<pointer>(__end_));
       std::swap(__first_, __t.__first_);
       std::swap(__begin_, __t.__begin_);
       std::swap(__end_, __t.__end_);
-      std::swap(__end_cap_, __t.__end_cap_);
+      std::swap(__cap_, __t.__cap_);
     }
   }
   __alloc_traits::construct(__alloc_, std::__to_address(__begin_ - 1), std::forward<_Args>(__args)...);
@@ -472,20 +472,20 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 void __split_buffer<_Tp, _Allocator>::emplace_fron
 template <class _Tp, class _Allocator>
 template <class... _Args>
 _LIBCPP_CONSTEXPR_SINCE_CXX20 void __split_buffer<_Tp, _Allocator>::emplace_back(_Args&&... __args) {
-  if (__end_ == __end_cap_) {
+  if (__end_ == __cap_) {
     if (__begin_ > __first_) {
       difference_type __d = __begin_ - __first_;
       __d                 = (__d + 1) / 2;
       __end_              = std::move(__begin_, __end_, __begin_ - __d);
       __begin_ -= __d;
     } else {
-      size_type __c = std::max<size_type>(2 * static_cast<size_t>(__end_cap_ - __first_), 1);
+      size_type __c = std::max<size_type>(2 * static_cast<size_t>(__cap_ - __first_), 1);
       __split_buffer<value_type, __alloc_rr&> __t(__c, __c / 4, __alloc_);
       __t.__construct_at_end(move_iterator<pointer>(__begin_), move_iterator<pointer>(__end_));
       std::swap(__first_, __t.__first_);
       std::swap(__begin_, __t.__begin_);
       std::swap(__end_, __t.__end_);
-      std::swap(__end_cap_, __t.__end_cap_);
+      std::swap(__cap_, __t.__cap_);
     }
   }
   __alloc_traits::construct(__alloc_, std::__to_address(__end_), std::forward<_Args>(__args)...);
diff --git a/libcxx/include/__vector/vector.h b/libcxx/include/__vector/vector.h
index 6db202efb279b3..57ba79a6a8b92a 100644
--- a/libcxx/include/__vector/vector.h
+++ b/libcxx/include/__vector/vector.h
@@ -822,7 +822,7 @@ vector<_Tp, _Allocator>::__swap_out_circular_buffer(__split_buffer<value_type, a
   __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->__end_cap(), __v.__cap_);
   __v.__first_ = __v.__begin_;
   __annotate_new(size());
 }
@@ -852,7 +852,7 @@ vector<_Tp, _Allocator>::__swap_out_circular_buffer(__split_buffer<value_type, a
 
   std::swap(this->__begin_, __v.__begin_);
   std::swap(this->__end_, __v.__end_);
-  std::swap(this->__end_cap(), __v.__end_cap_);
+  std::swap(this->__end_cap(), __v.__cap_);
   __v.__first_ = __v.__begin_;
   __annotate_new(size());
   return __ret;
diff --git a/libcxx/include/deque b/libcxx/include/deque
index 55d0bf9311bbb7..ad667503489741 100644
--- a/libcxx/include/deque
+++ b/libcxx/include/deque
@@ -2073,7 +2073,7 @@ void deque<_Tp, _Allocator>::__add_front_capacity() {
     std::swap(__map_.__first_, __buf.__first_);
     std::swap(__map_.__begin_, __buf.__begin_);
     std::swap(__map_.__end_, __buf.__end_);
-    std::swap(__map_.__end_cap_, __buf.__end_cap_);
+    std::swap(__map_.__cap_, __buf.__cap_);
     __start_ = __map_.size() == 1 ? __block_size / 2 : __start_ + __block_size;
   }
   __annotate_whole_block(0, __asan_poison);
@@ -2150,7 +2150,7 @@ void deque<_Tp, _Allocator>::__add_front_capacity(size_type __n) {
     std::swap(__map_.__first_, __buf.__first_);
     std::swap(__map_.__begin_, __buf.__begin_);
     std::swap(__map_.__end_, __buf.__end_);
-    std::swap(__map_.__end_cap_, __buf.__end_cap_);
+    std::swap(__map_.__cap_, __buf.__cap_);
     __start_ += __ds;
   }
 }
@@ -2196,7 +2196,7 @@ void deque<_Tp, _Allocator>::__add_back_capacity() {
     std::swap(__map_.__first_, __buf.__first_);
     std::swap(__map_.__begin_, __buf.__begin_);
     std::swap(__map_.__end_, __buf.__end_);
-    std::swap(__map_.__end_cap_, __buf.__end_cap_);
+    std::swap(__map_.__cap_, __buf.__cap_);
     __annotate_whole_block(__map_.size() - 1, __asan_poison);
   }
 }
@@ -2275,7 +2275,7 @@ void deque<_Tp, _Allocator>::__add_back_capacity(size_type __n) {
     std::swap(__map_.__first_, __buf.__first_);
     std::swap(__map_.__begin_, __buf.__begin_);
     std::swap(__map_.__end_, __buf.__end_);
-    std::swap(__map_.__end_cap_, __buf.__end_cap_);
+    std::swap(__map_.__cap_, __buf.__cap_);
     __start_ -= __ds;
   }
 }

Copy link

github-actions bot commented Nov 8, 2024

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

@winner245 winner245 force-pushed the winner245/__split_buffer__cap_ branch from d49930c to cc8d787 Compare November 8, 2024 17:15
Copy link
Contributor

@philnik777 philnik777 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with green CI.

@ldionne
Copy link
Member

ldionne commented Nov 11, 2024

The bootstrapping build failures are legitimate (the other ones are not). They are caused by the fact that we store a __split_buffer inside deque and changing the name of the variable breaks the LLDB data formatters.

I think the following diff should fix it, but @Michael137 should chime in to let us know if LLDB generally tries to be more backwards-compatible than what this diff achieves:

diff --git a/lldb/examples/synthetic/libcxx.py b/lldb/examples/synthetic/libcxx.py
index b078a4eb2f63..f8534cf9fdc9 100644
--- a/lldb/examples/synthetic/libcxx.py
+++ b/lldb/examples/synthetic/libcxx.py
@@ -765,7 +765,7 @@ class stddeque_SynthProvider:
                 )
             else:
                 map_endcap = map_.GetChildMemberWithName(
-                    "__end_cap_"
+                    "__cap_"
                 ).GetValueAsUnsigned(0)
 
             # check consistency

@winner245
Copy link
Contributor Author

Thank you for your feedback and help, ldionne! The diff you mentioned seems like a reasonable solution. I will wait for @Michael137's confirmation. If necessary, I can make adjustments in the PR to ensure compatibility.

@Michael137
Copy link
Member

Thank you for your feedback and help, ldionne! The diff you mentioned seems like a reasonable solution. I will wait for @Michael137's confirmation. If necessary, I can make adjustments in the PR to ensure compatibility.

Yea we usually try to keep backward compatibility for a couple of years (or until it becomes hard to maintain). Here's an example of how you could support two layouts:

value = pair.GetChildMemberWithName("__value_")
if not value.IsValid():
# pre-r300140 member name
value = pair.GetChildMemberWithName("__first_")

I.e., call GetChildMemberWithName with the new name. If that doesn't work. Try with the old name. Let me know if you need help making this work

@winner245
Copy link
Contributor Author

Thank you for your feedback and suggestions, @Michael137! I appreciate your guidance on maintaining backward compatibility. I will implement the suggested approach by checking for both the new and old member names in the LLDB data formatters. Thanks again for your help.

@winner245 winner245 force-pushed the winner245/__split_buffer__cap_ branch from cc8d787 to 6e51b12 Compare November 12, 2024 21:47
@llvmbot llvmbot added the lldb label Nov 12, 2024
Copy link

github-actions bot commented Nov 12, 2024

✅ With the latest revision this PR passed the Python code formatter.

@winner245 winner245 force-pushed the winner245/__split_buffer__cap_ branch from 6e51b12 to ef9e36a Compare November 12, 2024 21:56
@winner245 winner245 force-pushed the winner245/__split_buffer__cap_ branch from ef9e36a to bead12b Compare November 12, 2024 22:11
@Michael137
Copy link
Member

Michael137 commented Nov 13, 2024

LLDB changes LGTM, thanks!

The tools/lldb-dap/evaluate/TestDAP_evaluate.py test failure is unrelated to this PR and has been failing on other PRs too. So feel free to merge if the libc++ changes are good to go

@philnik777 philnik777 merged commit c7df106 into llvm:main Nov 13, 2024
58 of 63 checks passed
@winner245 winner245 deleted the winner245/__split_buffer__cap_ branch November 13, 2024 16:39
Michael137 added a commit to swiftlang/llvm-project that referenced this pull request Jun 18, 2025
…ng changes

Cherry-picked the LLDB parts from:
```
commit c7df106
Author: Peng Liu <[email protected]>
Date:   Wed Nov 13 05:08:08 2024 -0500

    Unify naming of internal pointer members in std::vector and
std::__split_buffer (llvm#115517)
```

Addresses llvm#144555
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. lldb
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants