[libcxx-commits] [PATCH] D115598: [libc++][NFC] Remove goto from std::string

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone accepted this revision as: Quuxplusone.
Quuxplusone added a comment.

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.



================
Comment at: libcxx/include/string:2361
     traits_type::move(__p, __s, __n);
-    traits_type::assign(__p[__n], value_type());
-    __set_size(__n);
-    __invalidate_iterators_past(__n);
+    __null_terminate_at(__p, __n);
   } else {
----------------
This can `return __null_terminate_at(__p, __n)` too. Tail-call for the win.


================
Comment at: libcxx/include/string:3001
                     traits_type::move(__p + __pos + __n2, __p + __pos + __n1, __n_move);
-                    goto __finish;
+                    return __null_terminate_at(__p, __n2 - __n1);
                 }
----------------
Here and below: `__sz + __n2 - __n1`, right? I hope this caused some test failures! :)


================
Comment at: libcxx/include/string:3223
     _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);
+    __null_terminate_at(__get_pointer(), size() - 1);
 }
----------------
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.


================
Comment at: libcxx/include/string:1698-1699
+    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;
----------------
nilayvaish wrote:
> Quuxplusone wrote:
> > This comment seems obsolete; I have no idea what "check" it's talking about. Let's remove the comment.
> 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?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115598/new/

https://reviews.llvm.org/D115598