Skip to content

Conversation

@winner245
Copy link
Contributor

@winner245 winner245 commented Dec 19, 2024

This PR slightly simplifies the implementation of std::vector's emplace_back and __construct_at_end functions to make the code more concise while maintaining the same functionality.

  • emplace_back: The simplified version removes the unnecessary temporary pointer variable __end and directly manipulates member variable this->__end_.
  • __construct_at_end and __move_range: the simplified version removes unnecessary local variables __pos and __new_end, and makes the loop more straightforward.

@winner245 winner245 force-pushed the simplify-emplace_back branch from b7debda to 317e9e4 Compare December 19, 2024 12:04
@winner245 winner245 marked this pull request as ready for review December 19, 2024 13:52
@winner245 winner245 requested a review from a team as a code owner December 19, 2024 13:52
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Dec 19, 2024
@llvmbot
Copy link
Member

llvmbot commented Dec 19, 2024

@llvm/pr-subscribers-libcxx

Author: Peng Liu (winner245)

Changes

This PR slightly simplifies the implementation of std::vector's emplace_back and __construct_at_end functions to make the code more concise while maintaining the same functionality.

  • emplace_back: The simplified version removes the unnecessary temporary pointer variable __end and directly manipulates member variable this->__end_.
  • __construct_at_end: the simplified version removes unnecessary local variables __pos and __new_end, and makes the loop more straightforward.

Full diff: https://github.com/llvm/llvm-project/pull/120521.diff

1 Files Affected:

  • (modified) libcxx/include/__vector/vector.h (+8-14)
diff --git a/libcxx/include/__vector/vector.h b/libcxx/include/__vector/vector.h
index 6ba7ba7bcf724b..7ad209be02d4aa 100644
--- a/libcxx/include/__vector/vector.h
+++ b/libcxx/include/__vector/vector.h
@@ -879,9 +879,8 @@ vector<_Tp, _Allocator>::__recommend(size_type __new_size) const {
 template <class _Tp, class _Allocator>
 _LIBCPP_CONSTEXPR_SINCE_CXX20 void vector<_Tp, _Allocator>::__construct_at_end(size_type __n) {
   _ConstructTransaction __tx(*this, __n);
-  const_pointer __new_end = __tx.__new_end_;
-  for (pointer __pos = __tx.__pos_; __pos != __new_end; __tx.__pos_ = ++__pos) {
-    __alloc_traits::construct(this->__alloc_, std::__to_address(__pos));
+  for (; __tx.__pos_ != __tx.__new_end_; ++__tx.__pos_) {
+    __alloc_traits::construct(this->__alloc_, std::__to_address(__tx.__pos_));
   }
 }
 
@@ -895,9 +894,8 @@ template <class _Tp, class _Allocator>
 _LIBCPP_CONSTEXPR_SINCE_CXX20 inline void
 vector<_Tp, _Allocator>::__construct_at_end(size_type __n, const_reference __x) {
   _ConstructTransaction __tx(*this, __n);
-  const_pointer __new_end = __tx.__new_end_;
-  for (pointer __pos = __tx.__pos_; __pos != __new_end; __tx.__pos_ = ++__pos) {
-    __alloc_traits::construct(this->__alloc_, std::__to_address(__pos), __x);
+  for (; __tx.__pos_ != __tx.__new_end_; ++__tx.__pos_) {
+    __alloc_traits::construct(this->__alloc_, std::__to_address(__tx.__pos_), __x);
   }
 }
 
@@ -1101,16 +1099,12 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 inline
     void
 #endif
     vector<_Tp, _Allocator>::emplace_back(_Args&&... __args) {
-  pointer __end = this->__end_;
-  if (__end < this->__cap_) {
+  if (this->__end_ < this->__cap_)
     __construct_one_at_end(std::forward<_Args>(__args)...);
-    ++__end;
-  } else {
-    __end = __emplace_back_slow_path(std::forward<_Args>(__args)...);
-  }
-  this->__end_ = __end;
+  else
+    (void)__emplace_back_slow_path(std::forward<_Args>(__args)...);
 #if _LIBCPP_STD_VER >= 17
-  return *(__end - 1);
+  return *(this->__end_ - 1);
 #endif
 }
 

@winner245 winner245 force-pushed the simplify-emplace_back branch from 317e9e4 to 224797d Compare December 19, 2024 16:58
@winner245 winner245 changed the title [libc++] Simplify vector's emplace_back and __construct_at_end functions [libc++] Simplify vector's emplace_back, __move_range and __construct_at_end functions Dec 19, 2024
@winner245 winner245 force-pushed the simplify-emplace_back branch from 224797d to 2fd517e Compare December 19, 2024 17:10
@winner245 winner245 force-pushed the simplify-emplace_back branch from 2fd517e to 9a382e6 Compare December 19, 2024 22:51
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.

This basically reverts https://reviews.llvm.org/D82111 and https://reviews.llvm.org/D80588, which we shouldn't do unless we can show that the compiler got smarter.

@winner245
Copy link
Contributor Author

Thank you for providing the context and related PRs. After reviewing those PRs, I understand the objectives of the current code, and they indeed improve the performance. Therefore, I am closing this PR.

@winner245 winner245 closed this Dec 20, 2024
@winner245 winner245 deleted the simplify-emplace_back branch December 20, 2024 18:53
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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants