[libcxx-commits] [PATCH] D113013: [libc++] Implement P1072R10 (std::basic_string::resize_and_overwrite)

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone accepted this revision.
Quuxplusone added inline comments.


================
Comment at: libcxx/include/string:997
+      __resize_default_init(__n);
+      auto __data = data();
+      __erase_to_end(_VSTD::move(__op)(__data, __n));
----------------
Quuxplusone wrote:
> Mordante wrote:
> > Quuxplusone wrote:
> > > Mordante wrote:
> > > > Is there a reason to use variable instead of calling `data()` in `__erase_to_end`.
> > > > The LLVM coding convention doesn't like `auto` when the type isn't clear.
> > > Sadly yes, `OP`'s parameters are overspecified as lvalues, not prvalues. I've just requested an LWG issue number for this.
> > > https://eel.is/c++draft/basic.string#string.capacity-7
> > In that case once you have a number, can you provide it here. Then we can add a comment why this is required. (And clean it up once the issue is resolved.)
> @mzeren-vmw @ckennelly (paper authors), thoughts? Shall we even just go ahead and do the sane thing here, on the assumption that LWG will catch up to us? :)
The LWG issue for this is https://timsong-cpp.github.io/lwg-issues/3645
@philnik, I suggest that you land D113013 as-is (with the lvalue `__data`); but then (if you don't mind the extra work) I think you should immediately submit another PR that changes the offending line to `__erase_to_end(_VSTD::move(__op)(data(), +__n));` with a message something like "[libc++] resize_and_overwrite: Adopt the P/R for LWG3645."  I can't vouch for @ldionne, but personally //I'd// accept such a PR right away. :)


================
Comment at: libcxx/test/std/strings/basic.string/string.capacity/resize_and_overwrite.pass.cpp:31
+    LIBCPP_ASSERT(s.begin().base() == p);
+    assert(std::all_of(p, p + k, [](const auto ch) { return ch == 'a'; }));
+    std::fill(p + k, p + n, 'b');
----------------
Nit: remove `const`, or add `&`, depending on your intent


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113013