[libc++] Refactor __split_buffer to eliminate code duplication #114138

winner245 · 2024-10-29T22:23:17Z

This PR refactors the __split_buffer class to eliminate code duplication in the push_back and push_front member functions.

Motivation:
The lvalue and rvalue reference overloads of push_back share identical logic, which coincides with that of emplace_back. Similarly, the overloads of push_front also share identical logic but lack an emplace_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:

Modifying both overloads of push_back to call emplace_back.
Introducing a new emplace_front member function that encapsulates the logic of push_front, allowing both overloads of push_front to call it (The addition of emplace_front also avoids the inconsistency regarding the absence of emplace_front).

The refactoring results in reduced code duplication, improved maintainability, and enhanced readability.

llvmbot · 2024-10-29T22:23:54Z

@llvm/pr-subscribers-libcxx

Author: Peng Liu (winner245)

Changes

This PR refactors the __split_buffer class to eliminate code duplication in the push_back and push_front member functions.

Motivation:
The lvalue and rvalue reference overloads of push_back share identical logic, which coincides with that of emplace_back. Similarly, the overloads of push_front also share identical logic but lack an emplace_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:

Modifying both overloads of push_back to call emplace_back.
Introducing a new emplace_front member function that encapsulates the logic of push_front, allowing both overloads of push_front to call it (The addition of emplace_front also avoids the inconsistency regarding the absence of emplace_front).

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:

(modified) libcxx/include/__split_buffer (+12-55)

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>

github-actions · 2024-10-29T22:26:45Z

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

frederick-vs-ja · 2024-10-30T10:16:16Z

This follows up #113481. CC @ldionne @philnik777.

philnik777

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?

frederick-vs-ja · 2024-10-30T12:01:53Z

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 push_front overloads, taking const value_type& and value_type&& respectively, containing almost identical code. The newly introduced emplace_front makes the code only written once.

philnik777 · 2024-10-30T12:18:59Z

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 push_front with emplace_front (and the same for push_back).

winner245 · 2024-10-30T13:48:21Z

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 push_front implementation.

Considering philnik777's suggestion to replace any uses of __split_buffer::push_front and __split_buffer::push_back with their emplace_xxx equivalents, I reviewed the current usage of __split_buffer. It appears that it is primarily utilized by std::vector and std::deque. Therefore, the proposed changes based on philnik777's suggestion would be:

Remove the two overloads of __split_buffer::push_back and retain only __split_buffer::emplace_back.
Remove the two overloads of __split_buffer::push_front, while adding __split_buffer::emplace_front (noting that __split_buffer currently does not provide emplace_front).
Modify std::vector and std::deque to replace all instances of __split_buffer::push_front and __split_buffer::push_back with the emplace_xxx methods.

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.

frederick-vs-ja · 2024-10-31T10:48:40Z

Let's just replace all __split_buffer::push_{front,back}. The changes are still simple and clear to me.

It appears that it is primarily utilized by std::vector and std::deque.

That's all. It is not used elsewhere.

frederick-vs-ja · 2024-11-01T13:42:46Z

CI failures are unrelated (due to missed inclusion in unrelated headers). You can try to rebase now.

winner245 · 2024-11-01T13:52:38Z

Thanks. I'll proceed with rebase now.

…ack,front}

…114138) This PR refactors the `__split_buffer` class to eliminate code duplication in the `push_back` and `push_front` member functions. **Motivation:** The lvalue and rvalue reference overloads of `push_back` share identical logic, which coincides with that of `emplace_back`. Similarly, the overloads of `push_front` also share identical logic but lack an `emplace_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: 1. Modifying both overloads of `push_back` to call `emplace_back`. 2. Introducing a new `emplace_front` member function that encapsulates the logic of `push_front`, allowing both overloads of `push_front` to call it (The addition of `emplace_front` also avoids the inconsistency regarding the absence of `emplace_front`). The refactoring results in reduced code duplication, improved maintainability, and enhanced readability.

winner245 requested a review from a team as a code owner October 29, 2024 22:23

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

winner245 force-pushed the winner245/split_buffer branch from f65f8c2 to faae1ed Compare October 29, 2024 22:39

frederick-vs-ja approved these changes Oct 30, 2024

View reviewed changes

philnik777 requested changes Oct 30, 2024

View reviewed changes

winner245 force-pushed the winner245/split_buffer branch from faae1ed to c248816 Compare October 31, 2024 15:08

philnik777 approved these changes Oct 31, 2024

View reviewed changes

winner245 force-pushed the winner245/split_buffer branch from c248816 to 13eec16 Compare November 1, 2024 13:49

winner245 added 2 commits November 2, 2024 12:32

Refactor __split_buffer to eliminate code duplication

389d2b0

Replace all usages of __split_buffer::push_{back,front} by emplace_{b…

4a2ff6b

…ack,front}

winner245 force-pushed the winner245/split_buffer branch from 13eec16 to 4a2ff6b Compare November 2, 2024 16:33

frederick-vs-ja approved these changes Nov 4, 2024

View reviewed changes

philnik777 merged commit f1c341c into llvm:main Nov 4, 2024
62 checks passed

winner245 deleted the winner245/split_buffer branch November 8, 2024 15:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[libc++] Refactor __split_buffer to eliminate code duplication #114138

[libc++] Refactor __split_buffer to eliminate code duplication #114138

winner245 commented Oct 29, 2024

llvmbot commented Oct 29, 2024

github-actions bot commented Oct 29, 2024 •

edited

Loading

frederick-vs-ja commented Oct 30, 2024

philnik777 left a comment

frederick-vs-ja commented Oct 30, 2024

philnik777 commented Oct 30, 2024

winner245 commented Oct 30, 2024

frederick-vs-ja commented Oct 31, 2024

frederick-vs-ja commented Nov 1, 2024

winner245 commented Nov 1, 2024

[libc++] Refactor __split_buffer to eliminate code duplication #114138

[libc++] Refactor __split_buffer to eliminate code duplication #114138

Conversation

winner245 commented Oct 29, 2024

llvmbot commented Oct 29, 2024

github-actions bot commented Oct 29, 2024 • edited Loading

frederick-vs-ja commented Oct 30, 2024

philnik777 left a comment

Choose a reason for hiding this comment

frederick-vs-ja commented Oct 30, 2024

philnik777 commented Oct 30, 2024

winner245 commented Oct 30, 2024

frederick-vs-ja commented Oct 31, 2024

frederick-vs-ja commented Nov 1, 2024

winner245 commented Nov 1, 2024

github-actions bot commented Oct 29, 2024 •

edited

Loading