Skip to content

Fix capacity increase issue with shrink_to_fit for __split_buffer #117720

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

Merged
merged 1 commit into from
Nov 26, 2024

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