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

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

Quuxplusone requested changes to this revision.
Quuxplusone added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/string:1697-1704
+    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());
----------------
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.


================
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;
----------------
This comment seems obsolete; I have no idea what "check" it's talking about. Let's remove the comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115598