[libcxx-commits] [PATCH] D98573: [libc++] Remove most of the special logic for "noexcept iterators" in basic_string
Louis Dionne via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Mon Apr 12 11:40:30 PDT 2021
ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.
Sorry for taking so long, I had this open for a long time but I was struggling with the condition in `basic_string::append`.
I'd be curious to know how you stumbled upon this.
================
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)))
{
----------------
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.
================
Comment at: libcxx/include/string:2825-2829
+ if (__ip == size())
+ {
+ append(__first, __last);
+ return begin() + __ip;
+ }
----------------
I'm not sure I understand this special case. Does it only save us from using a temporary string in case we're doing self assignment (line 2836 below) *and* inserting at the end of the string? If so, I think it's fine to leave it out and not special-case it.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D98573/new/
https://reviews.llvm.org/D98573
More information about the libcxx-commits
mailing list