[libcxx-commits] [PATCH] D150831: [libc++] Implement ranges::ends_with
Konstantin Varlamov via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Thu Jun 1 16:12:02 PDT 2023
var-const requested changes to this revision.
var-const added a comment.
This revision now requires changes to proceed.
Thanks for working on this, and sorry about the slow review!
================
Comment at: libcxx/include/__algorithm/ranges_ends_with.h:58
+ if (__n2 > __n1)
+ return false;
+ std::advance(__first1, (__n1 - __n2));
----------------
Please add a blank line after this line to separate code into logical blocks.
================
Comment at: libcxx/include/__algorithm/ranges_ends_with.h:62
+ auto __unwrapped2 = std::__unwrap_range(std::move(__first2), std::move(__last2));
+ return std::__equal_impl(
+ std::move(__unwrapped1.first),
----------------
Question: why are we calling `__equal_impl` rather than `ranges::equal`?
================
Comment at: libcxx/include/__algorithm/ranges_ends_with.h:81
+ _Range1&& __range1, _Range2&& __range2, _Pred __pred = {}, _Proj1 __proj1 = {}, _Proj2 __proj2 = {}) const {
+ auto __n1 = ranges::distance(__range1);
+ auto __n2 = ranges::distance(__range2);
----------------
Can you just delegate to the non-range overload of `operator()`?
================
Comment at: libcxx/include/__algorithm/ranges_ends_with.h:85
+ return false;
+ auto __new_range = ranges::drop_view(ranges::ref_view(__range1), __n1 - __n2);
+ auto __unwrapped1 = std::__unwrap_range(ranges::begin(__new_range), ranges::end(__new_range));
----------------
I wonder if `subrange(ranges::next(ranges::begin(__range1), __n1 - __n2))` would be preferable to avoid having a dependency on `drop_view` (which seems comparatively more heavyweight). (Delegation would make this a non-issue, though)
================
Comment at: libcxx/include/__algorithm/ranges_ends_with.h:99
+} // namespace __ends_with
+inline namespace __cpo {
+inline constexpr auto ends_with = __ends_with::__fn{};
----------------
Nit: please add a blank line before this line.
================
Comment at: libcxx/include/__algorithm/ranges_ends_with.h:59
+ return false;
+ std::advance(__first1, (N1 - N2));
+ auto __unwrapped1 = std::__unwrap_range(std::move(__first1), std::move(__last1));
----------------
philnik wrote:
> philnik wrote:
> >
> Potential optimization: If they are bidirectional and a common range we can step through back to front instead of iterating over the whole range.
Looks like it's still `std::advance`.
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.ends_with/ranges.ends_with.pass.cpp:66
+constexpr void test_iterators() {
+ { // simply tests
+ {int a[] = {1, 2, 3, 4, 5, 6};
----------------
Ultranit: `s/simply/simple/`.
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.ends_with/ranges.ends_with.pass.cpp:67
+ { // simply tests
+ {int a[] = {1, 2, 3, 4, 5, 6};
+ int p[] = {5, 6};
----------------
Nit: please add a newline here after the opening brace.
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.ends_with/ranges.ends_with.pass.cpp:69
+ int p[] = {5, 6};
+ std::same_as<bool> decltype(auto) ret =
+ std::ranges::ends_with(Iter1(a), Sent1(Iter1(a + 6)), Iter2(p), Sent2(Iter2(p + 2)));
----------------
Optional: I think checking the return type once is enough (no need to do it in every test case). I'd do it in a separate test, focusing on just that.
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.ends_with/ranges.ends_with.pass.cpp:76
+ int p[] = {5, 6};
+ auto whole = std::ranges::subrange(Iter1(a), Sent1(Iter1(a + 6)));
+ auto suffix = std::ranges::subrange(Iter2(p), Sent2(Iter2(p + 2)));
----------------
Consider using `std::end` to avoid dealing with indexes.
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.ends_with/ranges.ends_with.pass.cpp:103
+ {
+ int a[] = {1, 2, 3, 4, 5, 6};
+ int p[] = {1, 2, 3, 4, 5, 6};
----------------
Since the inputs are deliberately the same between the range overload and the iterator overload, I'd move them out (throughout the file):
```
int a[] = {1, 2, 3, 4, 5, 6};
int p[] = {1, 2, 3, 4, 5, 6};
auto whole = std::ranges::subrange(Iter1(a), Sent1(Iter1(a + 6)));
auto suffix = std::ranges::subrange(Iter2(p), Sent2(Iter2(p + 6)));
{
bool ret = std::ranges::ends_with(whole.begin(), whole.end(), suffix.begin(), suffix.end());
assert(ret);
}
{
bool ret = std::ranges::ends_with(whole, suffix);
assert(ret);
}
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.ends_with/ranges.ends_with.pass.cpp:119
+
+ { // suffix is longer than range
+ {
----------------
Can we also check the case where the suffix is repeated several times within the input but not at the end, to make sure the algorithm disregard the suffix if it's not at the very end?
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.ends_with/ranges.ends_with.pass.cpp:124
+ std::same_as<bool> decltype(auto) ret =
+ std::ranges::ends_with(Iter1(a), Sent1(Iter1(a + 6)), Iter2(p), Sent2(Iter2(p + 8)));
+ assert(!ret);
----------------
Please also use `whole` and `suffix` variables for consistency. With these repetitive kind of tests, it's important to keep them as uniform as possible to make the differences stand out more.
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.ends_with/ranges.ends_with.pass.cpp:155
+
+ { // range has zero length
+ {
----------------
Can we also check with 1-element range and (separately) 1-element suffix?
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.ends_with/ranges.ends_with.pass.cpp:179
+ Iter1(a), Sent1(Iter1(a + 6)), Iter2(p), Sent2(Iter2(p + 2)), [](int l, int r) { return l > r; });
+ assert(!ret);
+ }
----------------
But this wouldn't return `true` using the default predicate either, right? In general, for these checks it's better to supply a predicate such that the algorithm returns `true` with the custom predicate but `false` otherwise (since there's only one way the algorithm could return true but many ways it could return false, this check is more specific).
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.ends_with/ranges.ends_with.pass.cpp:202
+ [](int i) { return i + 3; },
+ [](int i) { return i * 2; });
+ assert(ret);
----------------
Very optional: I often find `* -1` good for this sort of tests (it's easy to see the difference between a number and its negative counterpart).
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.ends_with/ranges.ends_with.pass.cpp:218
+constexpr bool test() {
+ types::for_each(types::forward_iterator_list<int*>{}, []<class I2>() {
+ types::for_each(types::forward_iterator_list<int*>{}, []<class I1>() {
----------------
Shouldn't we also test with `input_iterator`s?
================
Comment at: libcxx/test/std/algorithms/ranges_robust_against_differing_projections.pass.cpp:80
test(std::ranges::starts_with, in, in2, eq, proj1, proj2);
- // test(std::ranges::ends_with, in, in2, eq, proj1, proj2);
#endif
----------------
ZijunZhao wrote:
> philnik wrote:
> > Please update all the `robust_against_*` tests!
> Files like `ranges_robust_against_nonbool_predicates.pass.cpp`, they don't have/comment out any `ends_with` tests but they have some tests involve `mismatch`, `all_of` and so on. So I need to add `ends_with` tests right?
Yes, those test files might not have newer algorithms mentioned but they should be updated where applicable.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D150831/new/
https://reviews.llvm.org/D150831
More information about the libcxx-commits
mailing list