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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante 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:
> > 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.)


================
Comment at: libcxx/test/std/strings/basic.string/string.capacity/resize_and_overwrite.pass.cpp:86
+  test<char16_t>();
+  test<char32_t>();
+
----------------
Quuxplusone wrote:
> Mordante wrote:
> > Next time it would be better to make a helper function that calls all 5 variants, just to keep `main` simpler.
> I forget if I specifically requested the current status quo, but I very well might have — personally I like <10 LOC of code repetition in main //better// than a second layer of `test` functions. :)
I prefer the extra layer, especially due to the duplication for `static_assert`.
However I don't object against this version.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113013