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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
Comment at: libcxx/test/std/strings/basic.string/string.cons/substr_rvalue.pass.cpp:35
+    S substr(std::move(orig), pos);
+    LIBCPP_ASSERT(orig.__invariants());
+    LIBCPP_ASSERT(orig.empty());
----------------
Optional: consider moving the checks into a helper function to cut down on boilerplate.


================
Comment at: libcxx/test/std/strings/basic.string/string.cons/substr_rvalue.pass.cpp:53
+constexpr void
+test_string_pos_alloc(S orig, typename S::size_type pos, const typename S::allocator_type& alloc, S expected) {
+  if (pos <= orig.size()) {
----------------
Can we do some equivalent of `DisableAllocationGuard` in this test as well?


================
Comment at: libcxx/test/std/strings/basic.string/string.cons/substr_rvalue.pass.cpp:61
+#ifndef TEST_HAS_NO_EXCEPTIONS
+  else if (!std::is_constant_evaluated()) {
+    try {
----------------
I'd split the normal case and the exceptional case in two functions. These are very different concerns, and I find the current triple-check (if `pos` is not valid, and it's not constant-evaluated, and exceptions are enabled) a little overwhelming. Making the `expected` parameter double as essentially a boolean also doesn't feel very clean. This would also be clearer at the call site (I'd group all normal cases and all exception cases together, i.e. a bunch of `test_string_pos`, `test_string_pos_n`, etc., followed by a bunch of `test_string_pos_exception`, `test_string_pos_n_exception`, etc.

I know it will increase the number of `test_string_*` functions even more, but I think it's the lesser evil. The amount of code won't increase substantially, and the smaller functions will be easier to read.


================
Comment at: libcxx/test/std/strings/basic.string/string.cons/substr_rvalue.pass.cpp:118
+constexpr void test_string(const typename S::allocator_type& alloc) {
+  test_string_pos<S>(STR(""), 0, STR(""));
+  test_string_pos<S>(STR(""), 1, STR("exception"));
----------------
Question (not for this patch): do we still need these macros even in the latest language modes, or can this now be achieved by regular code?


================
Comment at: libcxx/test/std/strings/basic.string/string.ops/string_substr/substr_rvalue.pass.cpp:28
+  if (pos <= orig.size()) {
+    auto orig_string_copy = orig;
+    auto rlen             = std::min<size_t>(n, orig.size() - pos);
----------------
`orig_string_copy` seems unused?


================
Comment at: libcxx/test/std/strings/basic.string/string.ops/string_substr/substr_rvalue.pass.cpp:29
+    auto orig_string_copy = orig;
+    auto rlen             = std::min<size_t>(n, orig.size() - pos);
+    S str                 = std::move(orig).substr(pos, n);
----------------
What's the benefit of calculating `rlen`, given that the `str == expected` check would also compare lengths? If it's necessary to check the length explicitly for some reason, I think it should be an argument to this function.


================
Comment at: libcxx/test/std/strings/basic.string/string.ops/string_substr/substr_rvalue.pass.cpp:32
+    LIBCPP_ASSERT(orig.__invariants());
+    LIBCPP_ASSERT(str.__invariants());
+    assert(str.size() == rlen);
----------------
In the constructor test we don't check `__invariants()` on `str`, can you make this consistent?


================
Comment at: libcxx/test/std/strings/basic.string/string.ops/string_substr/substr_rvalue.pass.cpp:26
+    auto rlen             = std::min<size_t>(n, orig.size() - pos);
+    S str                 = std::move(orig).substr(pos, n);
+    LIBCPP_ASSERT(orig.__invariants());
----------------
philnik wrote:
> avogelsgesang wrote:
> > DisableAllocationGuard`?
> Same reason as above: an implementation is allowed to allocate.
The implementation is recommended to avoid allocation, though, and I think we want to check that we follow that.


================
Comment at: libcxx/test/support/count_new.h:439
     {
-        // Don't re-disable if already disabled.
-        if (globalMemCounter.disable_allocations == true) m_disabled = false;
-        if (m_disabled) globalMemCounter.disableAllocations();
+        if (!TEST_IS_CONSTANT_EVALUATED) {
+            // Don't re-disable if already disabled.
----------------
Why is this a no-op in consteval mode?


================
Comment at: libcxx/test/support/count_new.h:453
 
-    ~DisableAllocationGuard() {
+    TEST_CONSTEXPR_CXX20 ~DisableAllocationGuard() {
         release();
----------------
Why are the constexpr language versions different between member functions?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131668