[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 added a subscriber: ldionne.
var-const added inline comments.


================
Comment at: libcxx/include/string:1326
+    constexpr basic_string& append_range(_Range&& __range) {
+      insert_range(end(), std::forward<_Range>(__range));
+      return *this;
----------------
Original comment by @ldionne

> 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?


================
Comment at: libcxx/test/std/strings/basic.string/string.modifiers/string_replace/replace_with_range.pass.cpp:26
+
+constexpr bool test_constraints_replace_with_range() {
+  // Input range with the same value type.
----------------
Original comment by @ldionne 

> You are also missing a SFINAE test for the constraints of the method.


================
Comment at: libcxx/test/std/strings/basic.string/string.modifiers/string_replace/replace_with_range.pass.cpp:55
+// - cut size: empty / one-element / several elements / until the end;
+// - input range: empty / one-element / middle-sized / longer than SSO / longer than the current string capacity.
+
----------------
Original comment by @ldionne 

> `// - input range: empty / one-element / middle-sized / longer than SSO / longer than current string capacity.`


================
Comment at: libcxx/test/std/strings/basic.string/string.modifiers/string_replace/replace_with_range.pass.cpp:862
+
+constexpr void test_string_replace_with_range_rvalue_range() {
+  // Make sure that the input range can be passed by both an lvalue and an rvalue reference and regardless of constness.
----------------
Original comment by @ldionne 
> We don't seem to be testing that the range is taken as `Rng&&`, are we?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149832