[libcxx-commits] [PATCH] D113013: [libc++] Implement P1072R10 (std::basic_string::resize_and_overwrite)
Arthur O'Dwyer via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Wed Nov 3 13:07:30 PDT 2021
Quuxplusone requested changes to this revision.
Quuxplusone added a subscriber: ldionne.
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>; } {
----------------
philnik wrote:
> Quuxplusone wrote:
> > Line-break after `constexpr`, please.
> > Also, I certainly wouldn't complain if `s/_Operation/_Op/`.
> What do you mean by
>
> > Also, I certainly wouldn't complain if s/_Operation/_Op/.
>
> ?
"I wouldn't complain if X" means "I'm not //asking// for X, but I'd be pleased if X were to happen anyway." Okay, usually it means I'm asking for X. ;)
`s/foo/bar/` is awk/sed-speak for "replace `foo` with `bar`."
As usual, the further down in the review I got, the more strongly I liked `_Op` over `_Operation`.
================
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())
----------------
philnik wrote:
> Quuxplusone wrote:
> > 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."
> I think the paper is right. What would be the point of moving a raw pointer or a size_type?
The question is whether it is permissible for libc++ to reject
```
std::string s;
s.resize_and_overwrite(100, [](char *p, int& n) {
return 0;
});
```
or, vice versa, whether it is permissible for us to accept
```
std::string s;
s.resize_and_overwrite(100, [](char *p, int&& n) {
return 0;
});
```
That is, it's not so much about the machine code //generated// as it is about the value-categories of things.
================
Comment at: libcxx/test/std/strings/basic.string/string.capacity/resize_and_overwrite.pass.cpp:22-38
+class move_only_functor {
+public:
+ constexpr move_only_functor(Operation op) : op_{op} {}
+ move_only_functor(const move_only_functor&) = delete;
+ move_only_functor(move_only_functor&&) = default;
+ move_only_functor& operator=(const move_only_functor&) = delete;
+ move_only_functor& operator=(move_only_functor&&) = default;
----------------
For the ref-qualification/value-category issue, this simple non-template test will suffice; you can get rid of `move_only_functor`.
```
void test_value_categories() {
std::string s;
s.resize_and_overwrite(10, [](char*&, size_t&) { return 0; });
struct RefQual {
int operator()(char *, size_t) && { return 0; }
};
s.resize_and_overwrite(10, RefQual());
}
```
(Here I'm following libstdc++ in assuming that `p, n` are passed as lvalues, even though I said above that they really ought to be passed as rvalues. If we hear that rvalues were intended, then just change `&` to `&&` above.)
================
Comment at: libcxx/test/std/strings/basic.string/string.capacity/resize_and_overwrite.pass.cpp:89-94
+ auto func = [&](auto* begin, size_t size) {
+ assert(size == new_size);
+ std::copy(s.begin(), s.end(), begin);
+ std::copy(s.begin(), s.begin() + s.size() / 2, begin + s.size());
+ return size;
+ };
----------------
Throughout: I see your reason for using the names `begin, size` for `Op`'s parameters, but I think it will be clearer to use `p, n` the same way the standard does. Also, speaking of `s.begin()` and `s.size()`, it looks like you're assuming things about their values without asserting those things. Could we assert them instead?
```
constexpr void test_undersized(S src, S orig)
{
S dst = orig;
size_t new_size = 1.5 * src.size();
auto func = [&](auto *p, size_t n) {
assert(n == new_size);
assert(dst.size() == new_size);
p = std::copy_n(src.begin(), src.size(), p);
std::copy_n(src.begin(), src.size() / 2, p);
return n;
};
dst.resize_and_overwrite(new_size, func);
assert(dst == src + src.substr(0, src.size() / 2);
}
```
But, writing it out this way, I just don't see the point of this test. What is `orig` doing there (your former `s3`)? Why does your `func` overwrite not just the new space but the original contents of the string too? What is the significance of the division in `size() / 2` as opposed to using a constant like `42`?
Could `test_undersized` and `test_oversized` both be replaced with something like...?
```
template<class S>
constexpr void test_appending(int k, int N) {
assert(N > k);
auto s = S(k, 'a');
s.resize_and_overwrite(N, [&](char *p, int n) {
assert(n == N);
LIBCPP_ASSERT(s.size() == n);
LIBCPP_ASSERT(s.begin() == p);
assert(std::all_of(p, p+k, [](auto ch) { return ch == 'a'; }));
LIBCPP_ASSERT(p[k] == '\0');
std::fill(p+k, p+n, 'b');
p[n] = 'c'; // will be overwritten
return n;
});
auto expected = S(k, 'a') + S(N-k, 'b');
assert(s == expected);
}
template<class S>
constexpr void test_truncating(int o, int N) {
assert(N < o);
auto s = S(o, 'a');
s.resize_and_overwrite(N, [&](char *p, int n) {
assert(n == N);
LIBCPP_ASSERT(s.size() == n);
LIBCPP_ASSERT(s.begin() == p);
assert(std::all_of(p, p+n, [](auto ch) { return ch == 'a'; }));
LIBCPP_ASSERT(p[n] == 'a');
p[n-1] = 'b';
p[n] = 'c'; // will be overwritten
return n;
});
auto expected = S(N-1, 'a') + 'b';
assert(s == expected);
}
```
However, this is still missing any coverage for what happens when `op(p, n) < n`. We should have coverage for that.
================
Comment at: libcxx/test/std/strings/basic.string/string.capacity/resize_and_overwrite.pass.cpp:107-112
+template <class S>
+constexpr void test_all(S s1, S s2) {
+ test_exact_size(s1, s2);
+ test_oversized(s1, s2);
+ test_undersized(s1, s2);
+}
----------------
Throughout: IIUC, `s1` means `dest` and `s2` means `src`. (On line 87, `s` means `dest` and `s3` means `src`. And so on.) Please consistent-ize the parameter names.
================
Comment at: libcxx/test/std/strings/basic.string/string.capacity/resize_and_overwrite.pass.cpp:123
+
+template <class S>
+constexpr bool test_overwrite_str() {
----------------
`template<class CharT>` — it's not a `S`tring.
Likewise on line 114 — `class CharT, class S` — although in that case, `S` is invariably `basic_string<CharT>`, so I don't see why it needs to be a parameter at all. Just `using S = basic_string<CharT>` in the body of the function is good enough.
================
Comment at: libcxx/test/std/strings/basic.string/string.capacity/resize_and_overwrite.pass.cpp:132-134
+#define CONSTEXPR_TEST(...) \
+ __VA_ARGS__; \
+ static_assert(__VA_ARGS__)
----------------
(Moot) These lines are too long.
(1) Unless @ldionne asked for this (in which case his preference can overrule mine), I'd much much prefer to see the usual libc++ style of `test_x(); static_assert(test_x());` written out longhand. It's only 10 lines of tests (as opposed to what you have here: 5 lines of tests plus 3 lines of macros).
(2) I don't understand why `test_overwrite_str` calls `test_copy_strings` calls `test_all` calls `test_{exact_size, oversized, undersized}`. These names don't seem to reflect any sort of hierarchy. I think it should just be that `test<CharT>` calls all the different tests, period.
(3) Only the one function that you're `static_assert`'ing on needs to `return true`; the rest should have return type `void`. (I suggest that that one bool-returning function should be named `test<CharT>()`.)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D113013/new/
https://reviews.llvm.org/D113013
More information about the libcxx-commits
mailing list