[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:1558
+#if _LIBCPP_STD_VER >= 23
+  template <_ContainerCompatibleRange<_CharT> _Range>
+  _LIBCPP_HIDE_FROM_ABI
----------------
I might have asked for that already, but do you have tests that these constraints are right, i.e. this overload doesn't match if you pass something that's not `_ContainerCompatibleRange`?


================
Comment at: libcxx/include/string:2661
+basic_string<_CharT, _Traits, _Allocator>::__assign_trivial(_Iterator __first, _Sentinel __last, size_type __n) {
+    _LIBCPP_ASSERT(__string_is_trivial_iterator<_Iterator>::value,
+                   "The iterator type given to `__assign_trivial` must be trivial");
----------------
You'll need to rebase on top of `main` and this should change to `_LIBCPP_ASSERT_UNCATEGORIZED`.


================
Comment at: libcxx/test/std/strings/basic.string/string.cons/from_range.pass.cpp:12
+// template<container-compatible-range<T> R>
+//   basic_string(from_range_t, R&& rg, const Allocator& = Allocator()); // C++23
+
----------------
This is marked as constexpr.


================
Comment at: libcxx/test/std/strings/basic.string/string.cons/from_range.pass.cpp:111
+int main(int, char**) {
+  for_all_iterators_and_allocators<char>([]<class Iter, class Sent, class Alloc>() {
+    // Shorter input -- SSO.
----------------
You don't seem to be testing this inside `constexpr`, but the constructor is definitely marked as `constexpr`. The same comment seems to apply to all tests.


================
Comment at: libcxx/test/std/strings/basic.string/string.modifiers/string_append/append_range.pass.cpp:62-63
+
+  // Note: `test_append_range_exception_safety_throwing_copy` doesn't apply because copying chars cannot throw.
+  test_string_append_range_exception_safety_throwing_allocator();
+
----------------
Can't we just inline this function here in a local scope? Applies to other tests as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149832