[libc++] Refactor basic_string::__recommend

[philnik777]

This does a couple of things:

• code that is only useful for shrink_to_fit is moved into that function
• shrink_to_fit is simplified a bit
• __recommend is renamed to better reflect what the function actually does
• __allocate_long_buffer asserts that the passed capacity doesn't fit into the SSO

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Nikolas Klauser (philnik777)

Changes

This does a couple of things:

• code that is only useful for shrink_to_fit is moved into that function
• shrink_to_fit is simplified a bit
• __recommend is renamed to better reflect what the function actually does
• __allocate_long_buffer asserts that the passed capacity doesn't fit into the SSO

---

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

1 Files Affected:

• (modified) libcxx/include/string (+10-12)

diff --git a/libcxx/include/string b/libcxx/include/string
index 363f27a51648c..f1e817a90c9ba 100644
--- a/libcxx/include/string
+++ b/libcxx/include/string
@@ -2246,7 +2246,9 @@ private:
   // Allocate a buffer of __capacity size with __alloc and return it
   _LIBCPP_HIDE_FROM_ABI static _LIBCPP_CONSTEXPR_SINCE_CXX20 __long
   __allocate_long_buffer(_Allocator& __alloc, size_type __capacity) {
-    auto __buffer = std::__allocate_at_least(__alloc, __recommend(__capacity) + 1);
+    _LIBCPP_ASSERT_INTERNAL(
+        !__fits_in_sso(__capacity), "Trying to allocate long buffer which fits into the small buffer");
+    auto __buffer = std::__allocate_at_least(__alloc, __align_allocation_size(__capacity) + 1);
 
     if (__libcpp_is_constant_evaluated()) {
       for (size_type __i = 0; __i != __buffer.count; ++__i)
@@ -2344,16 +2346,15 @@ private:
     return (__s + (__a - 1)) & ~(__a - 1);
   }
   enum { __alignment = 8 };
-  static _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 size_type __recommend(size_type __s) _NOEXCEPT {
-    if (__s < __min_cap) {
-      return static_cast<size_type>(__min_cap) - 1;
-    }
+
+  _LIBCPP_HIDE_FROM_ABI static _LIBCPP_CONSTEXPR_SINCE_CXX20 size_type
+  __align_allocation_size(size_type __size) _NOEXCEPT {
     const size_type __boundary = sizeof(value_type) < __alignment ? __alignment / sizeof(value_type) : __endian_factor;
-    size_type __guess          = __align_it<__boundary>(__s + 1) - 1;
+    size_type __guess          = __align_it<__boundary>(__size + 1) - 1;
     if (__guess == __min_cap)
       __guess += __endian_factor;
 
-    _LIBCPP_ASSERT_INTERNAL(__guess >= __s, "recommendation is below the requested size");
+    _LIBCPP_ASSERT_INTERNAL(__guess >= __size, "aligned allocation size is below the requested size");
     return __guess;
   }
 
@@ -3410,18 +3411,15 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 void basic_string<_CharT, _Traits, _Allocator>::re
 
 template <class _CharT, class _Traits, class _Allocator>
 inline _LIBCPP_CONSTEXPR_SINCE_CXX20 void basic_string<_CharT, _Traits, _Allocator>::shrink_to_fit() _NOEXCEPT {
-  size_type __target_capacity = __recommend(size());
-  if (__target_capacity == capacity())
+  if (!__is_long())
     return;
 
-  _LIBCPP_ASSERT_INTERNAL(__is_long(), "Trying to shrink small string");
-
   // We're a long string and we're shrinking into the small buffer.
   const auto __ptr  = __get_long_pointer();
   const auto __size = __get_long_size();
   const auto __cap  = __get_long_cap();
 
-  if (__fits_in_sso(__target_capacity)) {
+  if (__fits_in_sso(__align_allocation_size(size()))) {
     __annotation_guard __g(*this);
     __set_short_size(__size);
     traits_type::copy(std::__to_address(__get_short_pointer()), std::__to_address(__ptr), __size + 1);

[ldionne]

Why is __guess one unit larger than it was before?

[ldionne]

This is a great opportunity to add a comment explaining what this function does, and most interestingly, why it is needed. Perhaps some of that can be offloaded to the explanation of __endian_factor at the top of the class.

[ldionne]

Why do you not call __recommend (__align_allocation_size) anymore?

[ldionne]

Same here.

[ldionne]

IIUC we'll end up computing this again inside __allocate_long_buffer, right? That's a bit annoying, but perhaps the compiler's able to fold everything if it inlines __allocate_long_buffer. Did you check whether it does?