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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/string:888
+        __str.__default_init();
+        _Traits::move(data(), data() + __pos, __len);
+        __set_size(__len);
----------------
Mordante wrote:
> 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.
Do you mean if it's optimized out when the compiler knows that `__pos == 0`? Or do you mean it's fine as-is, since this scenario is optimized in `memmove`?


================
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());
+    }
----------------
Mordante wrote:
> 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.
I don't know why we have the non-conforming behaviour. I just didn't want to change the behaviour in this patch. I've filed https://llvm.org/PR57190.


================
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>
----------------
Mordante wrote:
> Can you improve these too?
what exactly are you asking for?


================
Comment at: libcxx/include/string:880
+    _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) {
----------------
var-const wrote:
> Other constructors also insert the newly-created string into the debug database, should this be done here as well?
Yes, I think so. I couldn't find any comprehensive tests for the debug mode though. Do you know if we have any/where they are?


================
Comment at: libcxx/include/string:885
+      auto __len = std::min(__n, __str.size() - __pos);
+      if (__alloc_traits::is_always_equal::value || __alloc == __str.__alloc()) {
+        __r_.first() = __str.__r_.first();
----------------
var-const wrote:
> Is this check necessary? Wouldn't `__alloc == __str.__alloc()` return `true` in that case?
Yes, but that allows the compiler to always remove the comparison.


================
Comment at: libcxx/include/string:868-869
 
     _LIBCPP_CONSTEXPR_AFTER_CXX17
-    basic_string(const basic_string& __str, size_type __pos, size_type __n,
-                 const _Allocator& __a = _Allocator());
+    basic_string(const basic_string& __str, size_type __pos, size_type __n, const _Allocator& __a = _Allocator());
     _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX17
----------------
huixie90 wrote:
> Does this need `_LIBCPP_HIDE_FROM_ABI`
No, this constructor is part of the ABI.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131668