[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.

2365

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

3008–3009

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

3251

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

3251

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

3251

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

3028

[• Quuxplusone]

libcxx/include/string

3008–3009

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

3028

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;
     }
 
+    void __finish_replace(size_type& __sz, size_type& __n1, size_type& __n2, value_type*& __p) {
+      // __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());
+    }
+
     _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();

 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);
   } 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>
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;
+                    __finish_replace(__sz, __n1, __n2, __p);
+                    return *this;
                 }
                 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());
+        __finish_replace(__sz, __n1, __n2, __p);
     }
     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>&
if (__is_long())
         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);
 }
 
 template <class _CharT, class _Traits, class _Allocator>
 inline
 void
 basic_string<_CharT, _Traits, _Allocator>::clear() _NOEXCEPT
 {
     __invalidate_all_iterators();