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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/test/std/strings/basic.string/string.cons/substr_rvalue.pass.cpp:53
+    try {
+      [[maybe_unused]] S substr = S(std::move(orig), pos);
+      assert(false);
----------------
avogelsgesang wrote:
> is it intentional, that this call doesn't pas the `alloc` parameter?
Nope, good catch!


================
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");
+
----------------
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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131668