[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 as: Quuxplusone.
Quuxplusone added a comment.

LGTM at this point, mod minor comments.



================
Comment at: libcxx/include/string:336-337
 
+    template<class Operation>
+    constexpr void resize_and_overwrite(size_type n, Operation op); // since C++23
+
----------------
Please move this up to between `resize` and `capacity`, to match https://eel.is/c++draft/basic.string
...er, I guess we have some pre-existing issues in that ordering. Either just move this up to between `resize` and `reserve`, or else redistribute our synopses of `reserve`-thru-`empty` to match the Standard's synopsis ordering. (But I wouldn't recommend anything more drastic than that.)


================
Comment at: libcxx/include/string:2765
 template <class _CharT, class _Traits, class _Allocator>
-inline void
+inline void _LIBCPP_CONSTEXPR_AFTER_CXX17
 basic_string<_CharT, _Traits, _Allocator>::__append_default_init(size_type __n)
----------------
Here and line 3495, since we're changing it anyway, I'd prefer to see
```
_LIBCPP_CONSTEXPR_AFTER_CXX17 void
```
We shouldn't need the `inline` keyword at all, because this is a template; and `constexpr void` reads better than `void constexpr`.
https://quuxplusone.github.io/blog/2021/04/03/static-constexpr-whittling-knife/


================
Comment at: libcxx/test/std/strings/basic.string/string.capacity/resize_and_overwrite.pass.cpp:23
+template <class S>
+constexpr void test_appending(size_t k, size_t N, size_t resizeSize) {
+  assert(N > k);
----------------
Neat. Perhaps `resizeSize` should be named `newCapacity`? (The ultimate //size// of the final string will be `N`.)


================
Comment at: libcxx/test/std/strings/basic.string/string.capacity/resize_and_overwrite.pass.cpp:61
+  test_appending<S>(10, 15, 15);
+  test_appending<S>(10, 15, 20);
+  test_appending<S>(10, 40, 40);
----------------
I think you're missing an interesting case: something like `test_appending<S>(10, 30, 15)`, where the original string fits in SSO, then we need to allocate for the lambda call, and then the final string would again fit in SSO (but IIUC we do not actually put it //back// into SSO — we just leave it with the extra allocated capacity, right? This is fine).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113013