[libcxx-commits] [PATCH] D131668: [libc++] Implement P2438R2 (std::string::substr() &&)

[Adrian Vogelsgesang via Phabricator via libcxx-commits]

avogelsgesang accepted this revision.
avogelsgesang added a comment.

LGTM % one more nit



================
Comment at: libcxx/include/string:879
+    _LIBCPP_HIDE_FROM_ABI constexpr
+    basic_string(basic_string&& __str, size_type __pos, size_type __n, const _Allocator& __alloc = _Allocator())
+        : __r_(__default_init_tag(), __alloc) {
----------------
philnik wrote:
> avogelsgesang wrote:
> > So far, all constructors and functions were defined outside the interface definition. Was there a conscious decision to no longer do so? If not, I think we should keep implementations out of the class definitions for consistency.
> That's not true. See `basic_string(__uininitialized_size_tag, size_type, const allocator_type&)` for example. I don't actually know why the function definitions were written out of line. It just leads to massive amounts of boiler-plate.
ok, your call


================
Comment at: libcxx/test/std/strings/basic.string/string.cons/substr_rvalue.pass.cpp:136
+  test_string_pos_n<S>("long long string so no SSO", 26, 10, "");
+  test_string_pos_n<S>("long long string so no SSO", 27, 10, "exception");
+
----------------
philnik wrote:
> avogelsgesang wrote:
> > philnik wrote:
> > > avogelsgesang wrote:
> > > > until here, the calls don't use the provided `alloc`. Still, they are executed multiple times as part of the `test_string` invocations with varying allocators. Maybe split this function into a `test_string_without_allocator` and `test_string_with_allocator` and call `test_string_without_allocator` only once from `test`
> > > These functions test with a default-constructed allocator.
> > I know. But the code
> > 
> > ```
> >   test_string<std::string>(std::allocator<char>{});
> >   test_string<std::basic_string<char, std::char_traits<char>, min_allocator<char>>>(min_allocator<char>{});
> >   test_string<std::basic_string<char, std::char_traits<char>, test_allocator<char>>>(test_allocator<char>{42});
> > ```
> > 
> > below leads to the `test_string_pos_n` function being called 3 times, with the exact same parameters, given that they only rely on the default constructed allocator and ignore the `alloc` passed to `test_string`. That seems unnecessarily repetitive
> Running the tests with different allocators ensures that the allocator gets properly default-constructed. `std::allocator` doesn't have any state, so nobody would notice if the allocator wasn't constructed at all. It's also always a good idea to give templates user-defined types to ensure that the library doesn't just work with library-defined types. 
makes sense! Thanks for the explanation


================
Comment at: libcxx/test/std/strings/basic.string/string.ops/string_substr/substr.pass.cpp:11
 
-// basic_string substr(size_type pos = 0, size_type n = npos) const; // constexpr since C++20
+// basic_string substr(size_type pos = 0, size_type n = npos) const; // constexpr since C++20, const & since C++23
 
----------------
also split into two functions here, same as in the `string` synopsis


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131668