[libcxx-commits] [PATCH] D98573: [libc++] Remove the special logic for "noexcept iterators" in basic_string

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

Quuxplusone marked 4 inline comments as done.
Quuxplusone added a comment.

> I remember you had a guard that would put back '\0' at the end of the string if the operation failed in a previous version of this patch.

That guard was too naïve; it failed to deal correctly with https://quuxplusone.github.io/blog/2021/04/17/pathological-string-appends/#self-appending-via-conversion-operators
The guard could reliably "undo" the unzeroing of the `\0`; but the problem was that //during the iteration// the user might ask to look at that `\0` again, and if its value had already changed, then boom. It's not enough to "plan to undo our change to the `\0`"; we must actually //never change the `\0`// (until the very last step). So, no more guard.



================
Comment at: libcxx/include/string:640
+struct __string_is_trivial_iterator<_Tp*>
+    : public is_arithmetic<_Tp> {};
 
----------------
ldionne wrote:
> I don't understand why only pointers to arithmetic types are considered trivial here?
We need to exclude value_types like `struct Evil`; i.e., the permissible value_types are non-class-types-convertible-to-`_CharT`. In practice I figured that boils down to "arithmetic types," since other non-class value_types (e.g. `int*`) won't be convertible to `_CharT` in the first place.
We could get cleverer with something like `_BOOL_CONSTANT<!is_class<_Tp>::value>`, but that's "cleverness" (which is how we got into the mess) and vastly diminishing returns.


================
Comment at: libcxx/include/string:2493
+    if (__string_is_trivial_iterator<_ForwardIterator>::value &&
+        (__cap >= __n || !__addr_in_range(*__first)))
     {
----------------
ldionne wrote:
> You can't dereference `__first` here if `__first == __last`.
If we get to that subexpression, we know that `__string_is_trivial_iterator<_ForwardIterator>::value` (so `__n` is meaningful) and `!(__cap >= __n)` (so `__n > __cap`), so we know `__n > 0`, so we know `__first != __last`.

(FYI: We //are// still probably being non-conforming by dereferencing `__first` both here and below. I think we're mandated to evaluate `*it` only once for each `it` in the range. But that was a pre-existing issue, and I'd like to land this "bugfix" patch before worrying about the quantum-leap-simplification to a different algorithm.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98573