[libc++][NFC] Remove goto from std::string

[philnik]

Summary

Remove goto from std::string

[• Quuxplusone]

libcxx/include/string

1697–1704

We could make this

basic_string<_CharT, _Traits, _Allocator>& __null_terminate_at(value_type *__p, size_type __newsz)
{
    __set_size(__newsz);
    __invalidate_iterators_past(__newsz);
    traits_type::assign(__p[__newsz], value_type());
    return *this;
}

and then change about 10 callers to the appropriate choice out of

return __null_terminate_at(__p, __n);
__null_terminate_at(__get_pointer(), __n);
__null_terminate_at(__p, __sz + __n);
return __null_terminate_at(__p, __sz + (__n2 - __n1));

etc. The only friction I see to that is that some of these callers, e.g. append and insert, are currently skipping the call to __invalidate_iterators_past(__sz) because they know there aren't any valid iterators past the new size. So either we pessimize them in debug mode, or more likely we just leave them as-is and don't upgrade them to this new helper function.

1698–1699

This comment seems obsolete; I have no idea what "check" it's talking about. Let's remove the comment.

[nilayvaish]

libcxx/include/string

1698–1699

Maybe the UBSAN annotation on line 2988 and 3038. I am guessing this will get sanitizer's attention now. Should we annotate this function with _LIBCPP_DISABLE_UBSAN_UNSIGNED_INTEGER_CHECK?

[philnik]

Applied Quuxplusone's suggestion

[nilayvaish]

libcxx/include/string

1697

The first param should be value_type* and update places where pointer is being to passed to pass value_type*.

[• Quuxplusone]

LGTM now, although it'd be nice to find the answer to my question about pop_back and __erase_to_end's history, just in case it matters.

libcxx/include/string

1698–1699

Thanks, that's definitely it! Instead of keeping the ubsan macro at all, though, I believe we could just change new line 3020 to avoid the overflow: return __null_terminate_at(__p, __sz - (__n1 - __n2)); because at that point __n1 >= __n2, unless I'm mistaken.

2361

This can return __null_terminate_at(__p, __n) too. Tail-call for the win.

3001

Here and below: __sz + __n2 - __n1, right? I hope this caused some test failures! :)

3222

Huh. Anyone got any idea why the left-hand code was so... hand-inlined? Anything in the git log that sheds light on this? Ditto __erase_to_end below.

[philnik]

• Replaced null_terminate_at() with erase_to_end() in pop_back()
• Added _LIBCPP_HIDE_FROM_ABI
• Fixed bugs

[nilayvaish]

libcxx/include/string

3222

I think we want to avoid checking whether the string is long or short again and again. Would likely save a few cycles.

[philnik]

libcxx/include/string

3222

This code is part of the initial libc++ commit. Unless you know where to get any (svn?) log, I think this question can't be answered. The only thing I could guess is some sort of compiler limitation 12+ years ago.

[ldionne]

LGTM with comment. Thanks for the refactoring, and happy this gets us closer to being constexpr friendly.

libcxx/include/string

3020

[• Quuxplusone]

libcxx/include/string

3001

Same issue as line 3020: either __sz + (__n2 - __n1) or __sz - __n1 + __n2.

3020

Or "match line 3023" should also work, I think: __sz - __n1 + __n2
The important thing is not to let __sz + __n2 be a subexpression, because it might wrap around, triggering that UBSan checker.

[philnik]

Added paranthesis

libcxx/include/string

inline basic_string& __assign_short(const value_type* __s, size_type __n) {
       pointer __p = __is_long()
                         ? (__set_long_size(__n), __get_long_pointer())
                         : (__set_short_size(__n), __get_short_pointer());
       traits_type::move(_VSTD::__to_address(__p), __s, __n);
       traits_type::assign(__p[__n], value_type());
       return *this;
     }
 
+    _LIBCPP_HIDE_FROM_ABI basic_string& __null_terminate_at(value_type* __p, size_type __newsz) {
+      __set_size(__newsz);
+      __invalidate_iterators_past(__newsz);
+      traits_type::assign(__p[__newsz], value_type());
+      return *this;
+    }
+
     _LIBCPP_INLINE_VISIBILITY void __invalidate_all_iterators();
     _LIBCPP_INLINE_VISIBILITY void __invalidate_iterators_past(size_type);
 
     template<class _Tp>
     _LIBCPP_INLINE_VISIBILITY
     bool __addr_in_range(_Tp&& __t) const {
         const volatile void *__p = _VSTD::addressof(__t);
         return data() <= __p && __p <= data() + size();
     }

 template <class _CharT, class _Traits, class _Allocator>
 basic_string<_CharT, _Traits, _Allocator>&
 basic_string<_CharT, _Traits, _Allocator>::__assign_external(
     const value_type* __s, size_type __n) {
   size_type __cap = capacity();
   if (__cap >= __n) {
     value_type* __p = _VSTD::__to_address(__get_pointer());
     traits_type::move(__p, __s, __n);
-    traits_type::assign(__p[__n], value_type());
-    __set_size(__n);
-    __invalidate_iterators_past(__n);
+    return __null_terminate_at(__p, __n);
   } else {
     size_type __sz = size();
     __grow_by_and_replace(__cap, __n - __cap, __sz, 0, __sz, __n, __s);
-  }
     return *this;
   }
+}
 
 template <class _CharT, class _Traits, class _Allocator>
 basic_string<_CharT, _Traits, _Allocator>&
 basic_string<_CharT, _Traits, _Allocator>::assign(const value_type* __s, size_type __n)
 {
     _LIBCPP_ASSERT(__n == 0 || __s != nullptr, "string::assign received nullptr");
     return (__builtin_constant_p(__n) && __n < __min_cap)
                ? __assign_short(__s, __n)
                : __assign_external(__s, __n);
 }
 
 template <class _CharT, class _Traits, class _Allocator>
 basic_string<_CharT, _Traits, _Allocator>&
 basic_string<_CharT, _Traits, _Allocator>::assign(size_type __n, value_type __c)
 {
     size_type __cap = capacity();
     if (__cap < __n)
     {
         size_type __sz = size();
         __grow_by(__cap, __n - __cap, __sz, 0, __sz);
     }
     value_type* __p = _VSTD::__to_address(__get_pointer());
     traits_type::assign(__p, __n, __c);
-    traits_type::assign(__p[__n], value_type());
-    __set_size(__n);
-    __invalidate_iterators_past(__n);
-    return *this;
+    return __null_terminate_at(__p, __n);
 }
 
 template <class _CharT, class _Traits, class _Allocator>
 basic_string<_CharT, _Traits, _Allocator>&
 basic_string<_CharT, _Traits, _Allocator>::operator=(value_type __c)
 {
     pointer __p;
     if (__is_long())
if (__cap - __sz + __n1 >= __n2)
         {
             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);
-                    goto __finish;
+                    return __null_terminate_at(__p, __sz + (__n2 - __n1));
                 }
                 if (__p + __pos < __s && __s < __p + __sz)
                 {
                     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);
-__finish:
-// __sz += __n2 - __n1; in this and the below function below can cause unsigned
-// integer overflow, but this is a safe operation, so we disable the check.
-        __sz += __n2 - __n1;
-        __set_size(__sz);
-        __invalidate_iterators_past(__sz);
-        traits_type::assign(__p[__sz], value_type());
+        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;
 }
 
 template <class _CharT, class _Traits, class _Allocator>
 basic_string<_CharT, _Traits, _Allocator>&
 basic_string<_CharT, _Traits, _Allocator>::replace(size_type __pos, size_type __n1, size_type __n2, value_type __c)
-    _LIBCPP_DISABLE_UBSAN_UNSIGNED_INTEGER_CHECK
 {
     size_type __sz = size();
     if (__pos > __sz)
         this->__throw_out_of_range();
     __n1 = _VSTD::min(__n1, __sz - __pos);
     size_type __cap = capacity();
     value_type* __p;
     if (__cap - __sz + __n1 >= __n2)
     {
         __p = _VSTD::__to_address(__get_pointer());
         if (__n1 != __n2)
         {
             size_type __n_move = __sz - __pos - __n1;
             if (__n_move != 0)
                 traits_type::move(__p + __pos + __n2, __p + __pos + __n1, __n_move);
         }
     }
     else
     {
         __grow_by(__cap, __sz - __n1 + __n2 - __cap, __sz, __pos, __n1, __n2);
         __p = _VSTD::__to_address(__get_long_pointer());
     }
     traits_type::assign(__p + __pos, __n2, __c);
-    __sz += __n2 - __n1;
-    __set_size(__sz);
-    __invalidate_iterators_past(__sz);
-    traits_type::assign(__p[__sz], value_type());
-    return *this;
+    return __null_terminate_at(__p, __sz - (__n1 - __n2));
 }
 
 template <class _CharT, class _Traits, class _Allocator>
 template<class _InputIterator>
 __enable_if_t
 <
     __is_cpp17_input_iterator<_InputIterator>::value,
     basic_string<_CharT, _Traits, _Allocator>&
basic_string<_CharT, _Traits, _Allocator>::__erase_external_with_move(
     if (__n)
     {
         size_type __sz = size();
         value_type* __p = _VSTD::__to_address(__get_pointer());
         __n = _VSTD::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);
-        __sz -= __n;
-        __set_size(__sz);
-        __invalidate_iterators_past(__sz);
-        traits_type::assign(__p[__sz], value_type());
+        __null_terminate_at(__p, __sz - __n);
     }
 }
 
 template <class _CharT, class _Traits, class _Allocator>
 basic_string<_CharT, _Traits, _Allocator>&
 basic_string<_CharT, _Traits, _Allocator>::erase(size_type __pos,
                                                  size_type __n) {
   if (__pos > size()) this->__throw_out_of_range();

 }
 
 template <class _CharT, class _Traits, class _Allocator>
 inline
 void
 basic_string<_CharT, _Traits, _Allocator>::pop_back()
 {
     _LIBCPP_ASSERT(!empty(), "string::pop_back(): string is already empty");
-    size_type __sz;
-    if (__is_long())
-    {
-        __sz = __get_long_size() - 1;
-        __set_long_size(__sz);
-        traits_type::assign(*(__get_long_pointer() + __sz), value_type());
-    }
-    else
-    {
-        __sz = __get_short_size() - 1;
-        __set_short_size(__sz);
-        traits_type::assign(*(__get_short_pointer() + __sz), value_type());
-    }
-    __invalidate_iterators_past(__sz);
+    __erase_to_end(size() - 1);
 }
 
 template <class _CharT, class _Traits, class _Allocator>
 inline
 void
 basic_string<_CharT, _Traits, _Allocator>::clear() _NOEXCEPT
 {
     __invalidate_all_iterators();
basic_string<_CharT, _Traits, _Allocator>::clear() _NOEXCEPT
     }
 }
 
 template <class _CharT, class _Traits, class _Allocator>
 inline
 void
 basic_string<_CharT, _Traits, _Allocator>::__erase_to_end(size_type __pos)
 {
-    if (__is_long())
-    {
-        traits_type::assign(*(__get_long_pointer() + __pos), value_type());
-        __set_long_size(__pos);
-    }
-    else
-    {
-        traits_type::assign(*(__get_short_pointer() + __pos), value_type());
-        __set_short_size(__pos);
-    }
-    __invalidate_iterators_past(__pos);
+  __null_terminate_at(_VSTD::__to_address(__get_pointer()), __pos);
 }
 
 template <class _CharT, class _Traits, class _Allocator>
 void
 basic_string<_CharT, _Traits, _Allocator>::resize(size_type __n, value_type __c)
 {
     size_type __sz = size();
     if (__n > __sz)