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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/test/std/containers/insert_range_helpers.h:31
+
+// A simple literal-type container. It can be used as a `constexpr` global variable (which isn't supported by
+// `std::vector`).
----------------
I think you don't need this class if you don't store your test cases in global variables. Then you could simply use `std::string` and it would work in `constexpr` too. IMO that would be better since this class is very specific.


================
Comment at: libcxx/test/std/containers/sequences/insert_range_sequence_containers.h:433
+    { // empty_c.insert_range(end, empty_range)
+      auto& test_case = EmptyContainer_EmptyRange<T>;
+
----------------
I think it makes sense to use a lambda to avoid repeating here like you do in the `replace_range_whatever` test for `string`.


================
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
+
----------------
We don't seem to be testing that the range is taken as `Rng&&`, are we?


================
Comment at: libcxx/test/std/strings/basic.string/string.modifiers/string_replace/replace_with_range.pass.cpp:31
+// Permutation matrix:
+// - initial string: empty / one-element / full;
+// - cut starts from: beginning / middle / end;
----------------
Maybe?


================
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.
+
----------------



================
Comment at: libcxx/test/std/strings/basic.string/string.modifiers/string_replace/replace_with_range.pass.cpp:491
+    Container& result = c.replace_with_range(from, to, in);
+    assert(result == c);
+    LIBCPP_ASSERT(c.__invariants());
----------------
We should really be returning a reference to the same object!


================
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() {
----------------
You are also missing a SFINAE test for the constraints of the method.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142335