[libc++][NFC] Use early returns in a few basic_string functions #137299

philnik777 · 2025-04-25T09:09:39Z

Using early returns tends to make the code easier to read, without any changes to the generated code.

llvmbot · 2025-07-19T07:42:18Z

@llvm/pr-subscribers-libcxx

Author: Nikolas Klauser (philnik777)

Changes

Using early returns tends to make the code easier to read, without any changes to the generated code.

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

1 Files Affected:

(modified) libcxx/include/string (+144-132)

diff --git a/libcxx/include/string b/libcxx/include/string
index 788af36d67c58..ae7ab7a81bee2 100644
--- a/libcxx/include/string
+++ b/libcxx/include/string
@@ -1414,19 +1414,21 @@ public:
     size_type __sz  = size();
     size_type __cap = capacity();
     size_type __n   = static_cast<size_type>(std::distance(__first, __last));
-    if (__n) {
-      if (__string_is_trivial_iterator_v<_ForwardIterator> && !__addr_in_range(*__first)) {
-        if (__cap - __sz < __n)
-          __grow_by_without_replace(__cap, __sz + __n - __cap, __sz, __sz, 0);
-        __annotate_increase(__n);
-        auto __end = __copy_non_overlapping_range(__first, __last, std::__to_address(__get_pointer() + __sz));
-        traits_type::assign(*__end, value_type());
-        __set_size(__sz + __n);
-      } else {
-        const basic_string __temp(__first, __last, __alloc_);
-        append(__temp.data(), __temp.size());
-      }
+    if (__n == 0)
+      return *this;
+
+    if (__string_is_trivial_iterator_v<_ForwardIterator> && !__addr_in_range(*__first)) {
+      if (__cap - __sz < __n)
+        __grow_by_without_replace(__cap, __sz + __n - __cap, __sz, __sz, 0);
+      __annotate_increase(__n);
+      auto __end = __copy_non_overlapping_range(__first, __last, std::__to_address(__get_pointer() + __sz));
+      traits_type::assign(*__end, value_type());
+      __set_size(__sz + __n);
+      return *this;
     }
+
+    const basic_string __temp(__first, __last, __alloc_);
+    append(__temp.data(), __temp.size());
     return *this;
   }
 
@@ -2776,24 +2778,23 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_NOINLINE basic_string<_CharT, _Traits, _Al
 basic_string<_CharT, _Traits, _Allocator>::__assign_no_alias(const value_type* __s, size_type __n) {
   const auto __cap  = __is_short ? static_cast<size_type>(__min_cap) : __get_long_cap();
   const auto __size = __is_short ? __get_short_size() : __get_long_size();
-  if (__n < __cap) {
-    if (__n > __size)
-      __annotate_increase(__n - __size);
-    pointer __p;
-    if (__is_short) {
-      __p = __get_short_pointer();
-      __set_short_size(__n);
-    } else {
-      __p = __get_long_pointer();
-      __set_long_size(__n);
-    }
-    traits_type::copy(std::__to_address(__p), __s, __n);
-    traits_type::assign(__p[__n], value_type());
-    if (__size > __n)
-      __annotate_shrink(__size);
-  } else {
+  if (__n >= __cap) {
     __grow_by_and_replace(__cap - 1, __n - __cap + 1, __size, 0, __size, __n, __s);
+    return *this;
   }
+
+  __annotate_delete();
+  auto __guard = std::__make_scope_guard(__annotate_new_size(*this));
+  pointer __p;
+  if (__is_short) {
+    __p = __get_short_pointer();
+    __set_short_size(__n);
+  } else {
+    __p = __get_long_pointer();
+    __set_long_size(__n);
+  }
+  traits_type::copy(std::__to_address(__p), __s, __n);
+  traits_type::assign(__p[__n], value_type());
   return *this;
 }
 
@@ -3007,52 +3008,56 @@ basic_string<_CharT, _Traits, _Allocator>::append(const value_type* __s, size_ty
   _LIBCPP_ASSERT_NON_NULL(__n == 0 || __s != nullptr, "string::append received nullptr");
   size_type __cap = capacity();
   size_type __sz  = size();
-  if (__cap - __sz >= __n) {
-    if (__n) {
-      __annotate_increase(__n);
-      value_type* __p = std::__to_address(__get_pointer());
-      traits_type::copy(__p + __sz, __s, __n);
-      __sz += __n;
-      __set_size(__sz);
-      traits_type::assign(__p[__sz], value_type());
-    }
-  } else
+  if (__cap - __sz < __n) {
     __grow_by_and_replace(__cap, __sz + __n - __cap, __sz, __sz, 0, __n, __s);
+    return *this;
+  }
+
+  if (__n == 0)
+    return *this;
+
+  __annotate_increase(__n);
+  value_type* __p = std::__to_address(__get_pointer());
+  traits_type::copy(__p + __sz, __s, __n);
+  __sz += __n;
+  __set_size(__sz);
+  traits_type::assign(__p[__sz], value_type());
   return *this;
 }
 
 template <class _CharT, class _Traits, class _Allocator>
 _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string<_CharT, _Traits, _Allocator>&
 basic_string<_CharT, _Traits, _Allocator>::append(size_type __n, value_type __c) {
-  if (__n) {
-    size_type __cap = capacity();
-    size_type __sz  = size();
-    if (__cap - __sz < __n)
-      __grow_by_without_replace(__cap, __sz + __n - __cap, __sz, __sz, 0);
-    __annotate_increase(__n);
-    pointer __p = __get_pointer();
-    traits_type::assign(std::__to_address(__p) + __sz, __n, __c);
-    __sz += __n;
-    __set_size(__sz);
-    traits_type::assign(__p[__sz], value_type());
-  }
+  if (__n == 0)
+    return *this;
+
+  size_type __cap = capacity();
+  size_type __sz  = size();
+  if (__cap - __sz < __n)
+    __grow_by_without_replace(__cap, __sz + __n - __cap, __sz, __sz, 0);
+  __annotate_increase(__n);
+  pointer __p = __get_pointer();
+  traits_type::assign(std::__to_address(__p) + __sz, __n, __c);
+  __sz += __n;
+  __set_size(__sz);
+  traits_type::assign(__p[__sz], value_type());
   return *this;
 }
 
 template <class _CharT, class _Traits, class _Allocator>
 _LIBCPP_CONSTEXPR_SINCE_CXX20 inline void
 basic_string<_CharT, _Traits, _Allocator>::__append_default_init(size_type __n) {
-  if (__n) {
-    size_type __cap = capacity();
-    size_type __sz  = size();
-    if (__cap - __sz < __n)
-      __grow_by_without_replace(__cap, __sz + __n - __cap, __sz, __sz, 0);
-    __annotate_increase(__n);
-    pointer __p = __get_pointer();
-    __sz += __n;
-    __set_size(__sz);
-    traits_type::assign(__p[__sz], value_type());
-  }
+  if (__n == 0)
+    return;
+  size_type __cap = capacity();
+  size_type __sz  = size();
+  if (__cap - __sz < __n)
+    __grow_by_without_replace(__cap, __sz + __n - __cap, __sz, __sz, 0);
+  __annotate_increase(__n);
+  pointer __p = __get_pointer();
+  __sz += __n;
+  __set_size(__sz);
+  traits_type::assign(__p[__sz], value_type());
 }
 
 template <class _CharT, class _Traits, class _Allocator>
@@ -3110,23 +3115,27 @@ basic_string<_CharT, _Traits, _Allocator>::insert(size_type __pos, const value_t
   if (__pos > __sz)
     this->__throw_out_of_range();
   size_type __cap = capacity();
-  if (__cap - __sz >= __n) {
-    if (__n) {
-      __annotate_increase(__n);
-      value_type* __p    = std::__to_address(__get_pointer());
-      size_type __n_move = __sz - __pos;
-      if (__n_move != 0) {
-        if (std::__is_pointer_in_range(__p + __pos, __p + __sz, __s))
-          __s += __n;
-        traits_type::move(__p + __pos + __n, __p + __pos, __n_move);
-      }
-      traits_type::move(__p + __pos, __s, __n);
-      __sz += __n;
-      __set_size(__sz);
-      traits_type::assign(__p[__sz], value_type());
-    }
-  } else
+
+  if (__cap - __sz < __n) {
     __grow_by_and_replace(__cap, __sz + __n - __cap, __sz, __pos, 0, __n, __s);
+    return *this;
+  }
+
+  if (__n == 0)
+    return *this;
+
+  __annotate_increase(__n);
+  value_type* __p    = std::__to_address(__get_pointer());
+  size_type __n_move = __sz - __pos;
+  if (__n_move != 0) {
+    if (std::__is_pointer_in_range(__p + __pos, __p + __sz, __s))
+      __s += __n;
+    traits_type::move(__p + __pos + __n, __p + __pos, __n_move);
+  }
+  traits_type::move(__p + __pos, __s, __n);
+  __sz += __n;
+  __set_size(__sz);
+  traits_type::assign(__p[__sz], value_type());
   return *this;
 }
 
@@ -3136,24 +3145,26 @@ basic_string<_CharT, _Traits, _Allocator>::insert(size_type __pos, size_type __n
   size_type __sz = size();
   if (__pos > __sz)
     this->__throw_out_of_range();
-  if (__n) {
-    size_type __cap = capacity();
-    value_type* __p;
-    if (__cap - __sz >= __n) {
-      __annotate_increase(__n);
-      __p                = std::__to_address(__get_pointer());
-      size_type __n_move = __sz - __pos;
-      if (__n_move != 0)
-        traits_type::move(__p + __pos + __n, __p + __pos, __n_move);
-    } else {
-      __grow_by_without_replace(__cap, __sz + __n - __cap, __sz, __pos, 0, __n);
-      __p = std::__to_address(__get_long_pointer());
-    }
-    traits_type::assign(__p + __pos, __n, __c);
-    __sz += __n;
-    __set_size(__sz);
-    traits_type::assign(__p[__sz], value_type());
+
+  if (__n == 0)
+    return *this;
+
+  size_type __cap = capacity();
+  value_type* __p;
+  if (__cap - __sz >= __n) {
+    __annotate_increase(__n);
+    __p                = std::__to_address(__get_pointer());
+    size_type __n_move = __sz - __pos;
+    if (__n_move != 0)
+      traits_type::move(__p + __pos + __n, __p + __pos, __n_move);
+  } else {
+    __grow_by_without_replace(__cap, __sz + __n - __cap, __sz, __pos, 0, __n);
+    __p = std::__to_address(__get_long_pointer());
   }
+  traits_type::assign(__p + __pos, __n, __c);
+  __sz += __n;
+  __set_size(__sz);
+  traits_type::assign(__p[__sz], value_type());
   return *this;
 }
 
@@ -3227,38 +3238,38 @@ basic_string<_CharT, _Traits, _Allocator>::replace(
     this->__throw_out_of_range();
   __n1            = std::min(__n1, __sz - __pos);
   size_type __cap = capacity();
-  if (__cap - __sz + __n1 >= __n2) {
-    value_type* __p = std::__to_address(__get_pointer());
-    if (__n1 != __n2) {
-      if (__n2 > __n1)
-        __annotate_increase(__n2 - __n1);
-      size_type __n_move = __sz - __pos - __n1;
-      if (__n_move != 0) {
-        if (__n1 > __n2) {
-          traits_type::move(__p + __pos, __s, __n2);
-          traits_type::move(__p + __pos + __n2, __p + __pos + __n1, __n_move);
-          return __null_terminate_at(__p, __sz + (__n2 - __n1));
-        }
-        if (std::__is_pointer_in_range(__p + __pos + 1, __p + __sz, __s)) {
-          if (__p + __pos + __n1 <= __s)
-            __s += __n2 - __n1;
-          else // __p + __pos < __s < __p + __pos + __n1
-          {
-            traits_type::move(__p + __pos, __s, __n1);
-            __pos += __n1;
-            __s += __n2;
-            __n2 -= __n1;
-            __n1 = 0;
-          }
-        }
+  if (__cap - __sz + __n1 < __n2) {
+    __grow_by_and_replace(__cap, __sz - __n1 + __n2 - __cap, __sz, __pos, __n1, __n2, __s);
+    return *this;
+  }
+
+  value_type* __p = std::__to_address(__get_pointer());
+  if (__n1 != __n2) {
+    if (__n2 > __n1)
+      __annotate_increase(__n2 - __n1);
+    size_type __n_move = __sz - __pos - __n1;
+    if (__n_move != 0) {
+      if (__n1 > __n2) {
+        traits_type::move(__p + __pos, __s, __n2);
         traits_type::move(__p + __pos + __n2, __p + __pos + __n1, __n_move);
+        return __null_terminate_at(__p, __sz + (__n2 - __n1));
+      }
+      if (std::__is_pointer_in_range(__p + __pos + 1, __p + __sz, __s)) {
+        if (__p + __pos + __n1 <= __s) {
+          __s += __n2 - __n1;
+        } else { // __p + __pos < __s < __p + __pos + __n1
+          traits_type::move(__p + __pos, __s, __n1);
+          __pos += __n1;
+          __s += __n2;
+          __n2 -= __n1;
+          __n1 = 0;
+        }
       }
+      traits_type::move(__p + __pos + __n2, __p + __pos + __n1, __n_move);
     }
-    traits_type::move(__p + __pos, __s, __n2);
-    return __null_terminate_at(__p, __sz + (__n2 - __n1));
-  } else
-    __grow_by_and_replace(__cap, __sz - __n1 + __n2 - __cap, __sz, __pos, __n1, __n2, __s);
-  return *this;
+  }
+  traits_type::move(__p + __pos, __s, __n2);
+  return __null_terminate_at(__p, __sz + (__n2 - __n1));
 }
 
 template <class _CharT, class _Traits, class _Allocator>
@@ -3311,15 +3322,16 @@ basic_string<_CharT, _Traits, _Allocator>::replace(size_type __pos, size_type __
 template <class _CharT, class _Traits, class _Allocator>
 _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_NOINLINE void
 basic_string<_CharT, _Traits, _Allocator>::__erase_external_with_move(size_type __pos, size_type __n) {
-  if (__n) {
-    size_type __sz     = size();
-    value_type* __p    = std::__to_address(__get_pointer());
-    __n                = std::min(__n, __sz - __pos);
-    size_type __n_move = __sz - __pos - __n;
-    if (__n_move != 0)
-      traits_type::move(__p + __pos, __p + __pos + __n, __n_move);
-    __null_terminate_at(__p, __sz - __n);
-  }
+  if (__n == 0)
+    return;
+
+  size_type __sz     = size();
+  value_type* __p    = std::__to_address(__get_pointer());
+  __n                = std::min(__n, __sz - __pos);
+  size_type __n_move = __sz - __pos - __n;
+  if (__n_move != 0)
+    traits_type::move(__p + __pos, __p + __pos + __n, __n_move);
+  __null_terminate_at(__p, __sz - __n);
 }
 
 template <class _CharT, class _Traits, class _Allocator>

libcxx/include/string

ldionne · 2025-08-12T16:27:35Z

libcxx/include/string

We probably don't want to optimize for the case where we're appending an empty string. Should we remove this special case? Let's look at the assembly.

ldionne · 2025-08-12T16:27:45Z

libcxx/include/string

ldionne · 2025-08-12T16:28:09Z

libcxx/include/string

Same comment.

ldionne · 2025-08-12T16:29:12Z

libcxx/include/string

In a follow-up PR, let's merge this into __resize_default_init since that's the only place where it is used. And actually __resize_default_init can also be inlined into resize_in_overwrite which is its only usage point.

ldionne · 2025-08-12T16:32:14Z

libcxx/include/string

Same comment. I suggest we tackle all of these removals in the same PR, as a follow-up to this one.

ldionne

LGTM but can you please run the string benchmarks before/after and ensure there's nothing that jumps out.

philnik777 · 2025-08-20T08:17:15Z

LGTM but can you please run the string benchmarks before/after and ensure there's nothing that jumps out.

Everything looked to be within the noise.

philnik777 force-pushed the string_early_return branch 2 times, most recently from 0560166 to 36e4bba Compare July 17, 2025 09:52

philnik777 marked this pull request as ready for review July 19, 2025 07:41

philnik777 requested a review from a team as a code owner July 19, 2025 07:41

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Jul 19, 2025

ldionne reviewed Aug 12, 2025

View reviewed changes

[libc++][NFC] Use early returns in a few basic_string functions

7a88b31

philnik777 force-pushed the string_early_return branch from 36e4bba to 7a88b31 Compare August 15, 2025 07:10

ldionne approved these changes Aug 19, 2025

View reviewed changes

philnik777 merged commit 2dc0a5f into llvm:main Aug 20, 2025
76 of 78 checks passed

philnik777 deleted the string_early_return branch August 20, 2025 08:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[libc++][NFC] Use early returns in a few basic_string functions #137299

[libc++][NFC] Use early returns in a few basic_string functions #137299

philnik777 commented Apr 25, 2025

Uh oh!

llvmbot commented Jul 19, 2025

Uh oh!

Uh oh!

Uh oh!

ldionne Aug 12, 2025

Uh oh!

ldionne Aug 12, 2025

Uh oh!

ldionne Aug 12, 2025

Uh oh!

ldionne Aug 12, 2025

Uh oh!

ldionne Aug 12, 2025

Uh oh!

ldionne left a comment

Uh oh!

Uh oh!

philnik777 commented Aug 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[libc++][NFC] Use early returns in a few basic_string functions #137299

[libc++][NFC] Use early returns in a few basic_string functions #137299

Conversation

philnik777 commented Apr 25, 2025

Uh oh!

llvmbot commented Jul 19, 2025

Uh oh!

Uh oh!

Uh oh!

ldionne Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

ldionne Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

ldionne Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

ldionne Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

ldionne Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

ldionne left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

philnik777 commented Aug 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants