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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added inline comments.
This revision is now accepted and ready to land.


================
Comment at: libcxx/include/string:2493
+    if (__string_is_trivial_iterator<_ForwardIterator>::value &&
+        (__cap >= __n || !__addr_in_range(*__first)))
     {
----------------
Quuxplusone wrote:
> 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.)
You're right in your analysis, I didn't take into account that you were short-circuiting the `||` if `n == 0`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98573