-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
[libc++] Refactor __split_buffer to eliminate code duplication #114138
base: main
Are you sure you want to change the base?
Conversation
@llvm/pr-subscribers-libcxx Author: Peng Liu (winner245) ChangesThis PR refactors the Motivation: Summary of Refactor:
The refactoring results in reduced code duplication, improved maintainability, and enhanced readability. Full diff: https://github.com/llvm/llvm-project/pull/114138.diff 1 Files Affected:
diff --git a/libcxx/include/__split_buffer b/libcxx/include/__split_buffer
index c4817601039f3b..99fe4059a0e15d 100644
--- a/libcxx/include/__split_buffer
+++ b/libcxx/include/__split_buffer
@@ -152,6 +152,8 @@ public:
_LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void push_front(value_type&& __x);
_LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void push_back(value_type&& __x);
+ template <class... _Args>
+ _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void emplace_front(_Args&&... __args);
template <class... _Args>
_LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void emplace_back(_Args&&... __args);
@@ -456,28 +458,17 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 void __split_buffer<_Tp, _Allocator>::shrink_to_fi
template <class _Tp, class _Allocator>
_LIBCPP_CONSTEXPR_SINCE_CXX20 void __split_buffer<_Tp, _Allocator>::push_front(const_reference __x) {
- if (__begin_ == __first_) {
- if (__end_ < __end_cap()) {
- difference_type __d = __end_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);
- __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());
- }
- }
- __alloc_traits::construct(__alloc(), std::__to_address(__begin_ - 1), __x);
- --__begin_;
+ emplace_front(__x);
}
template <class _Tp, class _Allocator>
_LIBCPP_CONSTEXPR_SINCE_CXX20 void __split_buffer<_Tp, _Allocator>::push_front(value_type&& __x) {
+ emplace_front(std::move(__x));
+}
+
+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_;
@@ -494,53 +485,19 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 void __split_buffer<_Tp, _Allocator>::push_front(v
std::swap(__end_cap(), __t.__end_cap());
}
}
- __alloc_traits::construct(__alloc(), std::__to_address(__begin_ - 1), std::move(__x));
+ __alloc_traits::construct(__alloc(), std::__to_address(__begin_ - 1), std::forward<_Args>(__args)...);
--__begin_;
}
template <class _Tp, class _Allocator>
_LIBCPP_CONSTEXPR_SINCE_CXX20 inline _LIBCPP_HIDE_FROM_ABI void
__split_buffer<_Tp, _Allocator>::push_back(const_reference __x) {
- if (__end_ == __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);
- __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());
- }
- }
- __alloc_traits::construct(__alloc(), std::__to_address(__end_), __x);
- ++__end_;
+ emplace_back(__x);
}
template <class _Tp, class _Allocator>
_LIBCPP_CONSTEXPR_SINCE_CXX20 void __split_buffer<_Tp, _Allocator>::push_back(value_type&& __x) {
- if (__end_ == __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);
- __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());
- }
- }
- __alloc_traits::construct(__alloc(), std::__to_address(__end_), std::move(__x));
- ++__end_;
+ emplace_back(std::move(__x));
}
template <class _Tp, class _Allocator>
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
f65f8c2
to
faae1ed
Compare
This follows up #113481. CC @ldionne @philnik777. |
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.
The other patch removed some code duplication, but this introduces emplace_front
and only uses it to implement push_front
. What is the point of that?
There're currently 2 |
Ah, I missed that. Given that we control all the uses of the type I think it'd be better to just replace any uses of |
Thank you, philnik777, for your insightful feedback. I also appreciate frederick-vs-ja's assistance in clarifying the purpose of my refactor, which effectively highlighted the code duplication issue with the current Considering philnik777's suggestion to replace any uses of
This approach seems to provide a more comprehensive refactor. However, I would like to hear @ldionne and @frederick-vs-ja's perspectives on this before proceeding. Thank you. |
This PR refactors the
__split_buffer
class to eliminate code duplication in thepush_back
andpush_front
member functions.Motivation:
The lvalue and rvalue reference overloads of
push_back
share identical logic, which coincides with that ofemplace_back
. Similarly, the overloads ofpush_front
also share identical logic but lack anemplace_front
member function, leading to an inconsistency. These identical internal logics lead to significant code duplication, making future maintenance more difficult.Summary of Refactor:
This PR reduces code redundancy by:
push_back
to callemplace_back
.emplace_front
member function that encapsulates the logic ofpush_front
, allowing both overloads ofpush_front
to call it (The addition ofemplace_front
also avoids the inconsistency regarding the absence ofemplace_front
).The refactoring results in reduced code duplication, improved maintainability, and enhanced readability.