Skip to content

[libcxx] Remove Redundant Reset in ~basic_string #164718

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+7 −3
Open

[libcxx] Remove Redundant Reset in ~basic_string #164718

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fd54b6a
[libcxx] Remove Redundant Reset in ~basic_string
boomanaiden154 Oct 22, 2025
3c58f67
refactor
boomanaiden154 Oct 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions libcxx/include/string
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() { __deallocate_buffer_if_long(); }

_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 operator __self_view() const _NOEXCEPT {
return __self_view(typename __self_view::__assume_valid(), data(), size());
Expand Down Expand Up @@ -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() {
Copy link
Member

@ldionne ldionne Oct 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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 --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?

Edit: Note that __uninitialized_tag already exists for no_destroy.h, we'd probably want to lift it somewhere else.

__annotate_delete();
if (__is_long())
__alloc_traits::deallocate(__alloc_, __get_long_pointer(), __get_long_cap());
}

// 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_buffer_if_long();
__rep_.__s = __short();
}

Expand Down
Loading