[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:1297
+    basic_string substr(size_type __pos = 0, size_type __n = npos) const& {
+      return basic_string(*this, __pos, __n, __alloc());
+    }
----------------
Mordante wrote:
> var-const wrote:
> > philnik wrote:
> > > var-const wrote:
> > > > Mordante wrote:
> > > > > 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.
> > > > Is the proposed resolution of LWG3752 to pass the allocator? I couldn't find the discussion it refers to, but because it says "we will fix it", I take it to mean that the current behavior, i.e., passing `__alloc()` should be changed, and the paper merely asks for confirmation.
> > > > 
> > > There is no officially proposed solution (as you can see). If you're asking if that's what the standard says, yes.
> > In that case, I feel we shouldn't pass `__alloc` for the rvalue overload -- yes, it would be inconsistent, but I don't think we should introduce more divergence and have more stuff to fix later. This is definitely something to check with @ldionne.
> For me it's fine to keep this since we already have this behaviour in our original `substr` code. So I don't mind waiting for the final resolution of LWG3752 before changing the code.
Since @ldionne mentioned on Discord LWG-3752 probably will be closed as NAD I agree more with @var-const.(I had expected LWG to mandate the current MSVC STL and libc++ implementation.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131668