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

[Adrian Vogelsgesang via Phabricator via libcxx-commits]

avogelsgesang added inline comments.


================
Comment at: libcxx/test/std/strings/basic.string/string.cons/substr_rvalue.pass.cpp:26
+  if (pos <= orig.size()) {
+    S substr(std::move(orig), pos);
+    LIBCPP_ASSERT(orig.__invariants());
----------------
philnik wrote:
> avogelsgesang wrote:
> > maybe use `DisableAllocationGuard` to check that this substring operation here doesn't allocate any memory?
> That's not actually guaranteed by the standard. An implementation is still allowed to allocate.
I know that the standard doesn't guarantee that. But your implementation intentionally avoids this allocation. As such, I think it makes sense to test this behavior to guard ourselves against unintended regressions. Maybe using a `LIBCPP_ASSERT`, so that other standard-conforming libraries still pass the test case?


================
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);
----------------
is it intentional, that this call doesn't pas the `alloc` parameter?


================
Comment at: libcxx/test/std/strings/basic.string/string.cons/substr_rvalue.pass.cpp:85
+  if (pos <= orig.size()) {
+    S substr(std::move(orig), pos, n, alloc);
+    LIBCPP_ASSERT(orig.__invariants());
----------------
philnik wrote:
> avogelsgesang wrote:
> > afaict, we have no coverage for the `S substr(std::move(orig), pos)` variant of the constructor
> Did you name the wrong constructor, or did you miss `test_string_pos`?
Indeed, I missed the constructor. All 4 variants are covered


================
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:
> > 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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131668