[libc++] Refactor __split_buffer to eliminate code duplication

[winner245]

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.

[llvmbot]

@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:

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.

---

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]

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

[frederick-vs-ja]

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]

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]

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]

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]

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]

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

[winner245]

Thanks. I'll proceed with rebase now.