-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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
Unify naming of internal pointer members in std::vector and std::__split_buffer #115517
Conversation
@llvm/pr-subscribers-lldb @llvm/pr-subscribers-libcxx Author: Peng Liu (winner245) ChangesRelated to PR #114423, this PR proposes to unify the naming of the internal pointer members in Both
This inconsistency between the names Full diff: https://github.com/llvm/llvm-project/pull/115517.diff 3 Files Affected:
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;
}
}
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
d49930c
to
cc8d787
Compare
There was a problem hiding this 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.
The bootstrapping build failures are legitimate (the other ones are not). They are caused by the fact that we store a 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 |
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: llvm-project/lldb/examples/synthetic/libcxx.py Lines 715 to 718 in 25d1ac1
I.e., call |
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. |
cc8d787
to
6e51b12
Compare
✅ With the latest revision this PR passed the Python code formatter. |
6e51b12
to
ef9e36a
Compare
ef9e36a
to
bead12b
Compare
LLDB changes LGTM, thanks! The |
…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
Related to PR #114423, this PR proposes to unify the naming of the internal pointer members in
std::vector
andstd::__split_buffer
for consistency and clarity.Both
std::vector
andstd::__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.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_
instd::__split_buffer
.