[libcxx-commits] [PATCH] D149832: [libc++][ranges] Implement the changes to `basic_string` from P1206 (`ranges::to`):

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const marked an inline comment as done.
var-const added inline comments.


================
Comment at: libcxx/include/string:1446
+    _LIBCPP_HIDE_FROM_ABI
+    constexpr iterator insert_range(const_iterator __position, _Range&& __range) {
+      if constexpr (ranges::forward_range<_Range> || ranges::sized_range<_Range>) {
----------------
ldionne wrote:
> `__string_is_trivial_iterator` was introduced around D98573. Do we have the same issue in the `insert_range` you're adding? Why do we not need to check for `__string_is_trivial_iterator` here?
> 
> I suspect there is an exception-safety issue hidden in the current version of the code, in which case we should add a test case similar to what was added in D98573 but for the new methods in this patch.
`insert_range` delegates to `__insert_with_size` which falls back to constructing a temporary string if the iterator is not trivial. I don't think there should be an exception safety issue, but the code is not symmetrical with `assign_range` now. Should I introduce something like `__insert_with_sentinel` to mirror the other function?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149832