Skip to content

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

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
Nov 13, 2024

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