Skip to content

Conversation

@winner245
Copy link
Contributor

This PR fixes the issue where __split_buffer::shrink_to_fit may unexpectedly increase the capacity, similar to the issue for std::vector in #97895. A demo to reproduce this problem is available. The fix follows the same approach used in #97895 for std::vector.

Note: An alternative approach would be simply remove this function. However, the current fix is as simple as removing it.

@winner245 winner245 force-pushed the fix-split-buffer-shrink_to_fit branch from b6fe9f9 to 956846a Compare November 26, 2024 15:18
@winner245 winner245 marked this pull request as ready for review November 26, 2024 17:56
@winner245 winner245 requested a review from a team as a code owner November 26, 2024 17:56
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Nov 26, 2024
@llvmbot
Copy link
Member

llvmbot commented Nov 26, 2024

@llvm/pr-subscribers-libcxx

Author: Peng Liu (winner245)

Changes

This PR fixes the issue where __split_buffer::shrink_to_fit may unexpectedly increase the capacity, similar to the issue for std::vector in #97895. A demo to reproduce this problem is available. The fix follows the same approach used in #97895 for std::vector.

Note: An alternative approach would be simply remove this function. However, the current fix is as simple as removing it.


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

1 Files Affected:

  • (modified) libcxx/include/__split_buffer (+8-6)
diff --git a/libcxx/include/__split_buffer b/libcxx/include/__split_buffer
index a44811c766735a..a637c83d17d107 100644
--- a/libcxx/include/__split_buffer
+++ b/libcxx/include/__split_buffer
@@ -410,12 +410,14 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 void __split_buffer<_Tp, _Allocator>::shrink_to_fi
     try {
 #endif // _LIBCPP_HAS_EXCEPTIONS
       __split_buffer<value_type, __alloc_rr&> __t(size(), 0, __alloc_);
-      __t.__construct_at_end(move_iterator<pointer>(__begin_), move_iterator<pointer>(__end_));
-      __t.__end_ = __t.__begin_ + (__end_ - __begin_);
-      std::swap(__first_, __t.__first_);
-      std::swap(__begin_, __t.__begin_);
-      std::swap(__end_, __t.__end_);
-      std::swap(__cap_, __t.__cap_);
+      if (__t.capacity() < capacity()) {
+        __t.__construct_at_end(move_iterator<pointer>(__begin_), move_iterator<pointer>(__end_));
+        __t.__end_ = __t.__begin_ + (__end_ - __begin_);
+        std::swap(__first_, __t.__first_);
+        std::swap(__begin_, __t.__begin_);
+        std::swap(__end_, __t.__end_);
+        std::swap(__cap_, __t.__cap_);
+      }
 #if _LIBCPP_HAS_EXCEPTIONS
     } catch (...) {
     }

Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense to me. It's not user-visible, but if deque::shrink_to_fit() aims to never increase the capacity, it feels like also doing the same for __split_buffer follows the design intent of deque::shrink_to_fit() better.

@ldionne ldionne merged commit 8458bbe into llvm:main Nov 26, 2024
64 checks passed
@winner245 winner245 deleted the fix-split-buffer-shrink_to_fit branch November 27, 2024 03:03
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