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

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/string:2662
+        if (__ptr_in_range(_VSTD::addressof(__tmp_ref), data(), data() + size()) ||
+            ((!__libcpp_is_trivial_iterator<_ForwardIterator>::value) && (__cap - __sz < __n)))
         {
----------------
Quuxplusone wrote:
> ldionne wrote:
> > Nit: I find the parens around `!__libcpp_is_trivial_iterator<_ForwardIterator>::value` confusing, are you OK with removing them?
> > 
> > Also, I would add a short comment saying something like
> > 
> > ```
> > // If we're appending from a range inside this string or if the iterators have non-trivial operations AND we need to allocate to complete the operation, make a temporary copy so we can provide the strong exception guarantee.
> > ```
> > 
> > ... or something like that. You're much better at English than I am, but I think a comment would be helpful since this line is quite packed with important information.
> Re parens: I prefer never to write `!x && y` because I believe people often thinko that when they meant `!(x && y)`. However, swapping them to give `(y && !x)` has no such issue, and in this case it's safe to write it that way, so I probably will do that.
> 
> Between this and your comment below, I think I need to revisit this patch and make sure //I// still understand the situations in which these codepaths are hit! :) I'll repost with more code comments.
> I'd be curious to know how you stumbled upon this.

I stumbled upon it in the process of answering https://stackoverflow.com/questions/66459162/where-do-standard-library-or-compilers-leverage-noexcept-move-semantics-other-t/66498981#66498981
I didn't actually hit the bug in real life; I just looked at the code and said "this is doing wacky complicated stuff with user-defined types, I guarantee it's wrong" and lo and behold, it was wrong. :)

This is also the second time this week I have an excuse to link to https://quuxplusone.github.io/blog/2018/06/12/attribute-noexcept-verify/ ;) in that the bug here is basically of the shape "I'm testing operation X for noexceptness, but then going and doing operation Y instead."


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98573