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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante requested changes to this revision.
Mordante added a comment.
This revision now requires changes to proceed.

Thanks for working on this!
Please mark the feature complete in `libcxx/docs/Status/Cxx2bPapers.csv`.

Please update `libcxx/utils/generate_feature_test_macro_components.py` with the new feature-test macro `__cpp_lib_string_resize_and_overwrite`; its values should be `202110L` and run this script to update the appropriate tests.

I didn't have enough time to do a thorough review; I'll have a closer look after all issues have been addressed.



================
Comment at: libcxx/include/string:160
     void resize(size_type n);
 
     void reserve(size_type res_arg);
----------------
Please update the synopsis.
```
    template<class Operation>
    constexpr void resize_and_overwrite(size_type n, Operation op); // since C++23
```


================
Comment at: libcxx/include/string:994
+      if (__n > capacity())
+          __shrink_or_extend(__recommend(__n));
+      __erase_to_end(__op(data(), __n));
----------------
This looks wrong. "Implementations should avoid unnecessary copies and allocations" This may allocate more than required. The paper `3. Implementation` refers to `__resize_default_init` as example.


================
Comment at: libcxx/test/std/strings/basic.string/string.capacity/resize_and_overwrite.pass.cpp:13
+
+// void resize_and_overwrite(size_type n, Operation op)
+
----------------
Please use the entire signature of the function.


================
Comment at: libcxx/test/std/strings/basic.string/string.capacity/resize_and_overwrite.pass.cpp:20
+constexpr void test_exact_size(S s) {
+  S s2;
+  s2.resize_and_overwrite(s.size() * 2, [&](char* begin, size_t size) {
----------------
All tests are now done on an empty string. Please also test with non-empty strings.


================
Comment at: libcxx/test/std/strings/basic.string/string.capacity/resize_and_overwrite.pass.cpp:64
+constexpr bool test() {
+  using S = std::string;
+  test_all<S>("");
----------------
It would be good to test with all string types. `libcxx/test/support/make_string.h` has a helper to create the same string for all supported string types. Then the test function can be templated on the string type.


================
Comment at: libcxx/test/std/strings/basic.string/string.capacity/resize_and_overwrite.pass.cpp:76
+
+int main() {
+  test();
----------------
Please use `int main(int, char**)`.


================
Comment at: libcxx/test/std/strings/basic.string/string.capacity/resize_and_overwrite.pass.cpp:79
+  static_assert(test());
+}
----------------
Please `return 0;` Some of our supported platforms require this even for `main`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113013