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

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

Quuxplusone added inline comments.


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


================
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);
+}
----------------
Mordante wrote:
> Can you test whether `s.c_str()` contains a proper NUL-terminated string. The same for the truncating test.
Ah, +1; I'd been implicitly assuming that `s == expected` would also check the null byte, but I guess it doesn't necessarily have to.
```
const S expected = S(k, 'a') + S(N - k, 'b');
assert(s == expected);
assert(s.c_str()[N] == '\0');
```


================
Comment at: libcxx/test/std/strings/basic.string/string.capacity/resize_and_overwrite.pass.cpp:86
+  test<char16_t>();
+  test<char32_t>();
+
----------------
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. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113013