[libcxx-commits] [libcxx] [libcxx] Remove Redundant Reset in ~basic_string (PR #164718)

[Louis Dionne via libcxx-commits]

================
@@ -2259,11 +2259,15 @@ private:
     return __long(__buffer, __capacity);
   }
 
-  // Deallocate the long buffer if it exists and clear the short buffer so we are an empty string
-  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 void __reset_internal_buffer() {
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 void __deallocate_buffer_if_long() {
----------------
ldionne wrote:

I would like to suggest the following solution, which solves the problem while IMO reducing the code complexity. It also addresses something that I didn't particularly like from the previous refactoring (the existence of `__replace_internal_buffer`). This makes the internal API closer in usage to `unique_ptr::reset`:

```diff
diff --git a/libcxx/include/string b/libcxx/include/string
index 8f80afbc2fd3..59d9722b40f4 100644
--- a/libcxx/include/string
+++ b/libcxx/include/string
@@ -1206,7 +1206,7 @@ public:
   }
 #  endif // _LIBCPP_CXX03_LANG
 
-  inline _LIBCPP_CONSTEXPR_SINCE_CXX20 ~basic_string() { __reset_internal_buffer(); }
+  inline _LIBCPP_CONSTEXPR_SINCE_CXX20 ~basic_string() { __reset_internal_buffer(__uninitialized_tag()); }
 
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 operator __self_view() const _NOEXCEPT {
     return __self_view(typename __self_view::__assume_valid(), data(), size());
@@ -2259,18 +2259,23 @@ private:
     return __long(__buffer, __capacity);
   }
 
-  // Deallocate the long buffer if it exists and clear the short buffer so we are an empty string
-  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 void __reset_internal_buffer() {
+  // Deallocate the long buffer if it exists. If __uninitialized_tag is passed, initialize neither
+  // the short nor the long representation. If a short or a long representation is passed, initialize
+  // the string with it.
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 void __reset_internal_buffer(__uninitialized_tag) {
     __annotate_delete();
     if (__is_long())
       __alloc_traits::deallocate(__alloc_, __get_long_pointer(), __get_long_cap());
-    __rep_.__s = __short();
   }
 
-  // Replace the current buffer with __alloc; the first __size elements constitute a string
-  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 void __replace_internal_buffer(__long __alloc) {
-    __reset_internal_buffer();
-    __rep_.__l = __alloc;
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 void __reset_internal_buffer(__short __contents) {
+    __reset_internal_buffer(__uninitialized_tag());
+    __rep_.__s = __contents;
+  }
+
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 void __reset_internal_buffer(__long __allocation) {
+    __reset_internal_buffer(__uninitialized_tag());
+    __rep_.__l = __allocation; // this function assumes that the first __size elements constitute a string
   }
 
   // Initialize the internal buffer to hold __size elements
@@ -2438,13 +2443,13 @@ private:
       __alloc_ = __str.__alloc_;
     else {
       if (!__str.__is_long()) {
-        __reset_internal_buffer();
+        __reset_internal_buffer(__short());
         __alloc_ = __str.__alloc_;
       } else {
         __annotate_delete();
         auto __guard = std::__make_scope_guard(__annotate_new_size(*this));
         auto __alloc = __str.__alloc_;
-        __replace_internal_buffer(__allocate_long_buffer(__alloc, __str.size()));
+        __reset_internal_buffer(__allocate_long_buffer(__alloc, __str.size()));
         __alloc_ = std::move(__alloc);
       }
     }
@@ -2655,7 +2660,7 @@ basic_string<_CharT, _Traits, _Allocator>::__init_with_sentinel(_InputIterator _
   __rep_ = __rep();
   __annotate_new(0);
 
-  auto __guard = std::__make_exception_guard([this] { __reset_internal_buffer(); });
+  auto __guard = std::__make_exception_guard([this] { __reset_internal_buffer(__short()); });
   for (; __first != __last; ++__first)
     push_back(*__first);
   __guard.__complete();
@@ -2675,7 +2680,7 @@ _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 void
 basic_string<_CharT, _Traits, _Allocator>::__init_with_size(_InputIterator __first, _Sentinel __last, size_type __sz) {
   pointer __p = __init_internal_buffer(__sz);
 
-  auto __guard = std::__make_exception_guard([this] { __reset_internal_buffer(); });
+  auto __guard = std::__make_exception_guard([this] { __reset_internal_buffer(__short()); });
   auto __end   = __copy_non_overlapping_range(std::move(__first), std::move(__last), std::__to_address(__p));
   traits_type::assign(*__end, value_type());
   __guard.__complete();
@@ -2710,7 +2715,7 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 void basic_string<_CharT, _Traits, _Allocator>::__
                       __sec_cp_sz);
   __buffer.__size_ = __n_copy + __n_add + __sec_cp_sz;
   traits_type::assign(__buffer.__data_[__buffer.__size_], value_type());
-  __replace_internal_buffer(__buffer);
+  __reset_internal_buffer(__buffer);
 }
 
 // __grow_by is deprecated because it does not set the size. It may not update the size when the size is changed, and it
@@ -2746,7 +2751,7 @@ _LIBCPP_DEPRECATED_("use __grow_by_without_replace") basic_string<_CharT, _Trait
   // This is -1 to make sure the caller sets the size properly, since old versions of this function didn't set the size
   // at all.
   __buffer.__size_ = -1;
-  __replace_internal_buffer(__buffer);
+  __reset_internal_buffer(__buffer);
 }
 
 template <class _CharT, class _Traits, class _Allocator>
@@ -2898,7 +2903,7 @@ basic_string<_CharT, _Traits, _Allocator>::__move_assign(basic_string& __str, tr
 {
   __annotate_delete();
   if (__is_long()) {
-    __reset_internal_buffer();
+    __reset_internal_buffer(__short());
 #    if _LIBCPP_STD_VER <= 14
     if (!is_nothrow_move_assignable<allocator_type>::value) {
       __set_short_size(0);
@@ -3394,7 +3399,7 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 void basic_string<_CharT, _Traits, _Allocator>::re
   __long __buffer  = __allocate_long_buffer(__alloc_, __requested_capacity);
   __buffer.__size_ = size();
   traits_type::copy(std::__to_address(__buffer.__data_), data(), __buffer.__size_ + 1);
-  __replace_internal_buffer(__buffer);
+  __reset_internal_buffer(__buffer);
 }
 
 template <class _CharT, class _Traits, class _Allocator>
@@ -3433,7 +3438,7 @@ inline _LIBCPP_CONSTEXPR_SINCE_CXX20 void basic_string<_CharT, _Traits, _Allocat
     }
 
     traits_type::copy(std::__to_address(__buffer.__data_), std::__to_address(__get_long_pointer()), __size + 1);
-    __replace_internal_buffer(__buffer);
+    __reset_internal_buffer(__buffer);
 #  if _LIBCPP_HAS_EXCEPTIONS
   } catch (...) {
     return;
```

Alternatively, we could decide that we're comfortable with making `__reset_internal_buffer()` equivalent to `__reset_internal_buffer(__short())` and we'd have to change fewer call sites. WDYT?

https://github.com/llvm/llvm-project/pull/164718