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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik marked 5 inline comments as done.
philnik added inline comments.


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


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


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


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


================
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());
----------------
avogelsgesang wrote:
> DisableAllocationGuard`?
Same reason as above: an implementation is allowed to allocate.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131668