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

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

Quuxplusone requested changes to this revision.
Quuxplusone added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/string:994
+    template <class _Operation>
+    _LIBCPP_HIDE_FROM_ABI constexpr void resize_and_overwrite(size_type __n, _Operation __op)
+        requires requires(_Operation __o) { {__o(pointer{}, size_type{})} -> _VSTD::convertible_to<size_type>; } {
----------------
Line-break after `constexpr`, please.
Also, I certainly wouldn't complain if `s/_Operation/_Op/`.


================
Comment at: libcxx/include/string:995
+    _LIBCPP_HIDE_FROM_ABI constexpr void resize_and_overwrite(size_type __n, _Operation __op)
+        requires requires(_Operation __o) { {__o(pointer{}, size_type{})} -> _VSTD::convertible_to<size_type>; } {
+      if (__n > capacity())
----------------
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p1072r10.html doesn't mention any //Constraints//, only a //Mandates// that `std::move(__o)(declval<_CharT*&>(), __n)` should have an integer-like type. I don't think "integer-like-ness" is expressible in C++, but prove me wrong. :)
Anyway, this function definitely should not be constrained, because (1) the standard says it shouldn't be, and (2) if we get our constraint wrong then we're needlessly preventing valid use-cases.

...Hm, I guess [p1072r10] has a minor wording bug, because obviously they intended to mandate the type of `std::move(op)(std::move(p), std::move(n))`, not `std::move(op)(p, n)`. I think we should "do what they mean, not what they say."


================
Comment at: libcxx/include/string:999
+
+      __erase_to_end(__op(data(), __n));
+    }
----------------
Forgot the `std::move` here. Please add a test case that catches this (e.g. use an `_Op` with a `&&`-ref-qualified `operator()`, and/or one where `operator()() &&` and `operator()() &` do different things).


================
Comment at: libcxx/test/std/strings/basic.string/string.capacity/resize_and_overwrite.pass.cpp:76
+
+int main() {
+  test();
----------------
Mordante wrote:
> Please use `int main(int, char**)`.
I question the usefulness of all of the "nonsense" strings. I understand `""` and `"Banane"` (i.e. `"short"`) and `"long long string so no SSO"`, but the other five seem calculated only to make the z/OS folks unhappy. ;)  If they're redundant, please eliminate them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113013