Reapply "[libc++] Simplify the implementation of reserve() and shrink_to_fit() (#113453)"

[philnik777]

The capacity is now passed correctly and a test for this path is added.

Since we changed the implementation of reserve(size_type) to only ever extend,
it doesn't make a ton of sense anymore to have __shrink_or_extend, since the code
paths of reserve and shrink_to_fit are now almost completely separate.

This patch splits up __shrink_or_extend so that the individual parts are in reserve
and shrink_to_fit depending on where they are needed.

This reverts commit 59f57be.

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 7ef636e1c4c49b175833dc91c44ed338b899c29b 32ccdb4c4c72c6adbc5ff4c00c5cd68c3ff8afe5 --extensions ,cpp -- libcxx/include/string libcxx/test/std/strings/basic.string/string.capacity/reserve_size.pass.cpp

View the diff from clang-format here.

diff --git a/libcxx/include/string b/libcxx/include/string
index 249b18bb77..b7f2d12269 100644
--- a/libcxx/include/string
+++ b/libcxx/include/string
@@ -3402,9 +3402,9 @@ inline _LIBCPP_CONSTEXPR_SINCE_CXX20 void basic_string<_CharT, _Traits, _Allocat
     return;
   }
 
-#if _LIBCPP_HAS_EXCEPTIONS
+#  if _LIBCPP_HAS_EXCEPTIONS
   try {
-#endif // _LIBCPP_HAS_EXCEPTIONS
+#  endif // _LIBCPP_HAS_EXCEPTIONS
     __annotation_guard __g(*this);
     auto __size       = size();
     auto __allocation = std::__allocate_at_least(__alloc_, __target_capacity + 1);
@@ -3423,11 +3423,11 @@ inline _LIBCPP_CONSTEXPR_SINCE_CXX20 void basic_string<_CharT, _Traits, _Allocat
     __alloc_traits::deallocate(__alloc_, __ptr, __get_long_cap());
     __set_long_cap(__allocation.count);
     __set_long_pointer(__allocation.ptr);
-#if _LIBCPP_HAS_EXCEPTIONS
+#  if _LIBCPP_HAS_EXCEPTIONS
   } catch (...) {
     return;
   }
-#endif // _LIBCPP_HAS_EXCEPTIONS
+#  endif // _LIBCPP_HAS_EXCEPTIONS
 }
 
 template <class _CharT, class _Traits, class _Allocator>

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Nikolas Klauser (philnik777)

Changes

The capacity is now passed correctly and a test for this path is added.

Since we changed the implementation of reserve(size_type) to only ever extend,
it doesn't make a ton of sense anymore to have __shrink_or_extend, since the code
paths of reserve and shrink_to_fit are now almost completely separate.

This patch splits up __shrink_or_extend so that the individual parts are in reserve
and shrink_to_fit depending on where they are needed.

This reverts commit 59f57be.

---

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

2 Files Affected:

• (modified) libcxx/include/string (+59-60)
• (modified) libcxx/test/std/strings/basic.string/string.capacity/reserve_size.pass.cpp (+9)

diff --git a/libcxx/include/string b/libcxx/include/string
index fdd8085106dcc60..b7f2d1226946392 100644
--- a/libcxx/include/string
+++ b/libcxx/include/string
@@ -1882,8 +1882,6 @@ public:
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 void __clear_and_shrink() _NOEXCEPT;
 
 private:
-  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 void __shrink_or_extend(size_type __target_capacity);
-
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_STRING_INTERNAL_MEMORY_ACCESS bool
   __is_long() const _NOEXCEPT {
     if (__libcpp_is_constant_evaluated() && __builtin_constant_p(__rep_.__l.__is_long_)) {
@@ -2079,6 +2077,21 @@ private:
 #  endif
   }
 
+  // Disable ASan annotations and enable them again when going out of scope. It is assumed that the string is in a valid
+  // state at that point, so `size()` can be called safely.
+  struct [[__nodiscard__]] __annotation_guard {
+    __annotation_guard(const __annotation_guard&)            = delete;
+    __annotation_guard& operator=(const __annotation_guard&) = delete;
+
+    _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 __annotation_guard(basic_string& __str) : __str_(__str) {
+      __str_.__annotate_delete();
+    }
+
+    _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 ~__annotation_guard() { __str_.__annotate_new(__str_.size()); }
+
+    basic_string& __str_;
+  };
+
   template <size_type __a>
   static _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 size_type __align_it(size_type __s) _NOEXCEPT {
     return (__s + (__a - 1)) & ~(__a - 1);
@@ -3357,7 +3370,16 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 void basic_string<_CharT, _Traits, _Allocator>::re
   if (__requested_capacity <= capacity())
     return;
 
-  __shrink_or_extend(__recommend(__requested_capacity));
+  __annotation_guard __g(*this);
+  auto __allocation = std::__allocate_at_least(__alloc_, __recommend(__requested_capacity) + 1);
+  auto __size       = size();
+  __begin_lifetime(__allocation.ptr, __allocation.count);
+  traits_type::copy(std::__to_address(__allocation.ptr), data(), __size + 1);
+  if (__is_long())
+    __alloc_traits::deallocate(__alloc_, __get_long_pointer(), __get_long_cap());
+  __set_long_cap(__allocation.count);
+  __set_long_size(__size);
+  __set_long_pointer(__allocation.ptr);
 }
 
 template <class _CharT, class _Traits, class _Allocator>
@@ -3366,69 +3388,46 @@ inline _LIBCPP_CONSTEXPR_SINCE_CXX20 void basic_string<_CharT, _Traits, _Allocat
   if (__target_capacity == capacity())
     return;
 
-  __shrink_or_extend(__target_capacity);
-}
+  _LIBCPP_ASSERT_INTERNAL(__is_long(), "Trying to shrink small string");
 
-template <class _CharT, class _Traits, class _Allocator>
-inline _LIBCPP_CONSTEXPR_SINCE_CXX20 void
-basic_string<_CharT, _Traits, _Allocator>::__shrink_or_extend(size_type __target_capacity) {
-  __annotate_delete();
-  auto __guard    = std::__make_scope_guard(__annotate_new_size(*this));
-  size_type __cap = capacity();
-  size_type __sz  = size();
-
-  pointer __new_data, __p;
-  bool __was_long, __now_long;
+  // We're a long string and we're shrinking into the small buffer.
   if (__fits_in_sso(__target_capacity)) {
-    __was_long = true;
-    __now_long = false;
-    __new_data = __get_short_pointer();
-    __p        = __get_long_pointer();
-  } else {
-    if (__target_capacity > __cap) {
-      // Extend
-      // - called from reserve should propagate the exception thrown.
-      auto __allocation = std::__allocate_at_least(__alloc_, __target_capacity + 1);
-      __new_data        = __allocation.ptr;
-      __target_capacity = __allocation.count - 1;
-    } else {
-      // Shrink
-      // - called from shrink_to_fit should not throw.
-      // - called from reserve may throw but is not required to.
+    __annotation_guard __g(*this);
+    auto __ptr  = __get_long_pointer();
+    auto __size = __get_long_size();
+    auto __cap  = __get_long_cap();
+    traits_type::copy(std::__to_address(__get_short_pointer()), std::__to_address(__ptr), __size + 1);
+    __set_short_size(__size);
+    __alloc_traits::deallocate(__alloc_, __ptr, __cap);
+    return;
+  }
+
 #  if _LIBCPP_HAS_EXCEPTIONS
-      try {
+  try {
 #  endif // _LIBCPP_HAS_EXCEPTIONS
-        auto __allocation = std::__allocate_at_least(__alloc_, __target_capacity + 1);
-
-        // The Standard mandates shrink_to_fit() does not increase the capacity.
-        // With equal capacity keep the existing buffer. This avoids extra work
-        // due to swapping the elements.
-        if (__allocation.count - 1 > capacity()) {
-          __alloc_traits::deallocate(__alloc_, __allocation.ptr, __allocation.count);
-          return;
-        }
-        __new_data        = __allocation.ptr;
-        __target_capacity = __allocation.count - 1;
+    __annotation_guard __g(*this);
+    auto __size       = size();
+    auto __allocation = std::__allocate_at_least(__alloc_, __target_capacity + 1);
+
+    // The Standard mandates shrink_to_fit() does not increase the capacity.
+    // With equal capacity keep the existing buffer. This avoids extra work
+    // due to swapping the elements.
+    if (__allocation.count - 1 > capacity()) {
+      __alloc_traits::deallocate(__alloc_, __allocation.ptr, __allocation.count);
+      return;
+    }
+
+    __begin_lifetime(__allocation.ptr, __allocation.count);
+    auto __ptr = __get_long_pointer();
+    traits_type::copy(std::__to_address(__allocation.ptr), std::__to_address(__ptr), __size + 1);
+    __alloc_traits::deallocate(__alloc_, __ptr, __get_long_cap());
+    __set_long_cap(__allocation.count);
+    __set_long_pointer(__allocation.ptr);
 #  if _LIBCPP_HAS_EXCEPTIONS
-      } catch (...) {
-        return;
-      }
+  } catch (...) {
+    return;
+  }
 #  endif // _LIBCPP_HAS_EXCEPTIONS
-    }
-    __begin_lifetime(__new_data, __target_capacity + 1);
-    __now_long = true;
-    __was_long = __is_long();
-    __p        = __get_pointer();
-  }
-  traits_type::copy(std::__to_address(__new_data), std::__to_address(__p), size() + 1);
-  if (__was_long)
-    __alloc_traits::deallocate(__alloc_, __p, __cap + 1);
-  if (__now_long) {
-    __set_long_cap(__target_capacity + 1);
-    __set_long_size(__sz);
-    __set_long_pointer(__new_data);
-  } else
-    __set_short_size(__sz);
 }
 
 template <class _CharT, class _Traits, class _Allocator>
diff --git a/libcxx/test/std/strings/basic.string/string.capacity/reserve_size.pass.cpp b/libcxx/test/std/strings/basic.string/string.capacity/reserve_size.pass.cpp
index 6196fd6836e7d16..cb1e6b18ed450d6 100644
--- a/libcxx/test/std/strings/basic.string/string.capacity/reserve_size.pass.cpp
+++ b/libcxx/test/std/strings/basic.string/string.capacity/reserve_size.pass.cpp
@@ -75,6 +75,15 @@ TEST_CONSTEXPR_CXX20 void test_string() {
     test<S>(100, 50, 1000);
     test<S>(100, 50, S::npos);
   }
+
+  { // Check that growing twice works as expected
+    S str;
+    str.reserve(50);
+    assert(str.capacity() >= 50);
+    size_t old_cap = str.capacity();
+    str.reserve(str.capacity() + 1);
+    assert(str.capacity() > old_cap);
+  }
 }
 
 TEST_CONSTEXPR_CXX20 bool test() {