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

[Adrian Vogelsgesang via Phabricator via libcxx-commits]

avogelsgesang added inline comments.


================
Comment at: libcxx/include/string:261
     size_type copy(value_type* s, size_type n, size_type pos = 0) const;                        // constexpr since C++20
-    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
+    constexpr basic_string substr(size_type pos = 0, size_type n = npos) &&; // since C++23
----------------
I would rather split this into two lines;

    basic_string substr(size_type pos = 0, size_type n = npos) const;                           // constexpr since C++20, removed in C++23
    basic_string substr(size_type pos = 0, size_type n = npos) const&;                           // since C++23


================
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) {
----------------
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.


================
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());
----------------
maybe use `DisableAllocationGuard` to check that this substring operation here doesn't allocate any memory?


================
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());
----------------
afaict, we have no coverage for the `S substr(std::move(orig), pos)` variant of the constructor


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


================
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());
----------------
DisableAllocationGuard`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131668