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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/string:1400-1402
+        size_type __n = __string_is_trivial_iterator<ranges::iterator_t<_Range>>::value ?
+            static_cast<size_type>(ranges::distance(__range)) : 0;
+        __assign_with_size(ranges::begin(__range), ranges::end(__range), __n);
----------------
We should refactor this so that `__assign_with_size` actually assigns with size, so we don't have to compute `__n = 0` when we don't actually need it anywhere. That's too confusing, it seems like we're assigning with size 0 when `!__string_is_trivial_iterator` but in reality `__assign_with_size` simply ignored the size we passed in.


================
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>) {
----------------
`__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.


================
Comment at: libcxx/test/std/strings/basic.string/string.cons/from_range.pass.cpp:20-21
+
+// template<container-compatible-range<T> R>
+//   basic_string(from_range_t, R&& rg, const Allocator& = Allocator()); // C++23
+
----------------
Above includes (here and elsewhere).


================
Comment at: libcxx/test/std/strings/basic.string/string.modifiers/string_append/append_range.pass.cpp:19
+
+void test_string_append_range_return_value() {
+  std::string c;
----------------
Here and in other tests, I would inline those functions into `main()` and instead do:

```
// plain english comment explaining what you test
{
  // test content
}
```

This is what we try to do elsewhere and a plain text comment allows for a more precise description of what's going on than a function name -- I'm not 100% sure what's being tested by just reading `test_string_append_range_return_value`.


================
Comment at: libcxx/test/std/strings/basic.string/string.modifiers/string_replace/size_size_string_size_size.pass.cpp:2
 //
+//===----------------------------------------------------------------------===//
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
----------------
I think this change is accidental.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149832