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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Looking at the paper I also noticed `4.5. Concerns on ABI stability`, did you verify whether this isn't an issue for us?



================
Comment at: libcxx/include/string:888
+        __str.__default_init();
+        _Traits::move(data(), data() + __pos, __len);
+        __set_size(__len);
----------------
philnik wrote:
> Mordante wrote:
> > Did you measure the overhead for this move when `__pos == 0`?
> I haven't measured, but AFAICT every memmove implementation is in that case basically a no-op. Our `char_traits::move` has `if (__s1 < __s2 ) {...} else if (__s2 < __s1) {...}`, the LLVM libc has `if (dst < src) ... else if (dst > src) ...` and musl has `if (d == s) return d;`. I haven't checked glibc, but I would be very surprised if if wasn't optimized there.
If this is optimized out I'm fine with it.


================
Comment at: libcxx/include/string:1297
+    basic_string substr(size_type __pos = 0, size_type __n = npos) const& {
+      return basic_string(*this, __pos, __n, __alloc());
+    }
----------------
philnik wrote:
> Mordante wrote:
> > This seems to be https://wg21.link/lwg3752, can you mention in the status page we implement this not yet accepted LWG-issue?
> I wouldn't consider LWG3752 to be implemented. It doesn't propose a solution and IMO the right thing would be to use `select_on_container_copy_construction` on the `const&` overload and passing the allocator on the `&&` overload.
But this deviates from the wording in the paper, there it's defined as
`return basic_string(*this, pos, n);` which uses `basic_string`'s default allocator.
So there should be some comments as why we have this non-conforming behaviour.


================
Comment at: libcxx/include/string:108
+    constexpr basic_string(basic_string&& str, size_type pos, const Allocator& a = Allocator());              // since C++23
+    constexpr basic_string(basic_string&& str, size_type pos, size_type n, const Allocator& a = Allocator()); // since C++23
     template<class T>
----------------
Can you improve these too?


================
Comment at: libcxx/test/std/strings/basic.string/string.cons/substr_rvalue.pass.cpp:185
+
+constexpr bool test2() {
+#ifndef TEST_HAS_NO_WIDE_CHARACTERS
----------------
Why `test1` and `test2` instead of just one `test`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131668