[libcxx-commits] [PATCH] D142335: [libc++][ranges] Implement `ranges::to`.

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const marked 4 inline comments as done.
var-const added inline comments.


================
Comment at: libcxx/include/string:1326
+    constexpr basic_string& append_range(_Range&& __range) {
+      return append(basic_string(from_range, std::forward<_Range>(__range), get_allocator()));
+    }
----------------
ldionne wrote:
> This one can be more efficient. If we have an input range, then we do need to make a temporary string and append it. But if we have e.g. a forward_range or a sized_range, we don't need to make a copy of the input range, and I think we should really avoid doing it. I think simply using `insert_range(end(), ...)` here would solve that problem?
Addressed in https://reviews.llvm.org/D149832.


================
Comment at: libcxx/test/std/strings/basic.string/string.modifiers/string_replace/replace_with_range.pass.cpp:13
+// template<container-compatible-range<charT> R>
+//   constexpr basic_string& replace_with_range(const_iterator i1, const_iterator i2, R&& rg); // C++23
+
----------------
ldionne wrote:
> We don't seem to be testing that the range is taken as `Rng&&`, are we?
Addressed in https://reviews.llvm.org/D149832.


================
Comment at: libcxx/test/std/strings/basic.string/string.modifiers/string_replace/replace_with_range.pass.cpp:34
+// - cut size: empty / one-element / several elements / until the end;
+// - input range: empty / one-element / middle-sized / long.
+
----------------
ldionne wrote:
> 
Addressed in https://reviews.llvm.org/D149832.


================
Comment at: libcxx/test/std/strings/basic.string/string.modifiers/string_replace/replace_with_range.pass.cpp:700
+}
+
+void test_string_replace_with_range_exception_safety_throwing_allocator() {
----------------
ldionne wrote:
> You are also missing a SFINAE test for the constraints of the method.
Addressed in https://reviews.llvm.org/D149832.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142335