[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