[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.

In general this looks good! I've some minor nit. It also seems you missed some of my "bookkeeping" remarks. I added these as comments.



================
Comment at: libcxx/include/string:336-337
 
+    template<class Operation>
+    constexpr void resize_and_overwrite(size_type n, Operation op); // since C++23
+
----------------
Quuxplusone wrote:
> 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.)
Basically where my original request for updating the synopsis is ;-)


================
Comment at: libcxx/include/string:337
+    template<class Operation>
+    constexpr void resize_and_overwrite(size_type n, Operation op); // since C++23
+
----------------
In my original reply I mentioned some not yet done things. For clarity I add them here to it's easier to tick the box once done.

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.




================
Comment at: libcxx/include/string:997
+      __resize_default_init(__n);
+      auto __data = data();
+      __erase_to_end(_VSTD::move(__op)(__data, __n));
----------------
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.


================
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);
----------------
Quuxplusone wrote:
> Neat. Perhaps `resizeSize` should be named `newCapacity`? (The ultimate //size// of the final string will be `N`.)
Also please use snake_case.


================
Comment at: libcxx/test/std/strings/basic.string/string.capacity/resize_and_overwrite.pass.cpp:36
+  });
+  const auto expected = S(k, 'a') + S(N - k, 'b');
+  assert(s == expected);
----------------
Please don't use `auto`.


================
Comment at: libcxx/test/std/strings/basic.string/string.capacity/resize_and_overwrite.pass.cpp:37
+  const auto expected = S(k, 'a') + S(N - k, 'b');
+  assert(s == expected);
+}
----------------
Can you test whether `s.c_str()` contains a proper NUL-terminated string. The same for the truncating test.


================
Comment at: libcxx/test/std/strings/basic.string/string.capacity/resize_and_overwrite.pass.cpp:86
+  test<char16_t>();
+  test<char32_t>();
+
----------------
Next time it would be better to make a helper function that calls all 5 variants, just to keep `main` simpler.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113013