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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/include/string:888
+        __str.__default_init();
+        _Traits::move(data(), data() + __pos, __len);
+        __set_size(__len);
----------------
philnik wrote:
> 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`?
I'm fine with this as is, so leave it to memmove. It gives some overhead by calling a function, but I don't think that's a big deal.


================
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:
> > 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.
I still would like a comment in the code mentioning this is non-standard behaviour and a link to the bug report.
That way when somebody sees this and wonders why it's clear this was done intentionally.


================
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>
----------------
philnik wrote:
> Mordante wrote:
> > Can you improve these too?
> what exactly are you asking for?
To split the line in two and align the `since C++23` with the other since comments.


================
Comment at: libcxx/test/std/strings/basic.string/string.cons/substr_rvalue.pass.cpp:185
+
+constexpr bool test2() {
+#ifndef TEST_HAS_NO_WIDE_CHARACTERS
----------------
philnik wrote:
> Mordante wrote:
> > Why `test1` and `test2` instead of just one `test`?
> As usual, constexpr step limit.
maybe add a comment, however I really love to increment the limit; but you're already seen that patch ;-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131668