[libcxx-commits] [PATCH] D150831: [libc++] Implement ranges::ends_with
Konstantin Varlamov via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Wed Aug 30 17:18:47 PDT 2023
var-const requested changes to this revision.
var-const added a comment.
This revision now requires changes to proceed.
Thanks a lot, this is getting very close to LGTM. The implementation looks good (just a few formatting nitpicks), just a few minor things in tests to address plus the benchmark needs to be reworked. My sincere apologies for the very slow review (mostly due to the upcoming LLVM 17 release).
================
Comment at: libcxx/include/__algorithm/ranges_ends_with.h:39
+ template <class _Iter1, class _Sent1, class _Iter2, class _Sent2, class _Pred, class _Proj1, class _Proj2>
+ static _LIBCPP_HIDE_FROM_ABI constexpr bool __bidirectional_impl(
+ _Iter1 __first1,
----------------
How about naming this function `__ends_with_fn_impl_bidirectional`, just for consistency with the other helpers?
================
Comment at: libcxx/include/__algorithm/ranges_ends_with.h:67
+ return __bidirectional_impl(__first1, __last1, __first2, __last2, __pred, __proj1, __proj2);
+ } else {
+ auto __n1 = ranges::distance(__first1, __last1);
----------------
Nit: please add a blank line before this line.
================
Comment at: libcxx/include/__algorithm/ranges_ends_with.h:73
+ if (__n2 > __n1)
+ return false;
+ return __ends_with_fn_impl_with_offset(
----------------
Nit: please add a blank line after this line.
================
Comment at: libcxx/include/__algorithm/ranges_ends_with.h:104
+ if constexpr (std::bidirectional_iterator<_Sent1> && std::bidirectional_iterator<_Sent2> &&
+ (!std::random_access_iterator<_Sent1>)&&(!std::random_access_iterator<_Sent2>)) {
+ return __bidirectional_impl(__first1, __last1, __first2, __last2, __pred, __proj1, __proj2);
----------------
Nit: can you add spaces before and after the `&&`? Or just `clang-format` these lines. If it's `clang-format` that produces this formatting, then never mind (although I'd be surprised).
================
Comment at: libcxx/include/__algorithm/ranges_ends_with.h:104
+ if constexpr (std::bidirectional_iterator<_Sent1> && std::bidirectional_iterator<_Sent2> &&
+ (!std::random_access_iterator<_Sent1>)&&(!std::random_access_iterator<_Sent2>)) {
+ return __bidirectional_impl(__first1, __last1, __first2, __last2, __pred, __proj1, __proj2);
----------------
var-const wrote:
> Nit: can you add spaces before and after the `&&`? Or just `clang-format` these lines. If it's `clang-format` that produces this formatting, then never mind (although I'd be surprised).
Nit: the parentheses around `!std::random_access_iterator<_Sent1>` are unnecessary (same for the `_Sent2` check).
================
Comment at: libcxx/include/__algorithm/ranges_ends_with.h:105
+ (!std::random_access_iterator<_Sent1>)&&(!std::random_access_iterator<_Sent2>)) {
+ return __bidirectional_impl(__first1, __last1, __first2, __last2, __pred, __proj1, __proj2);
+ } else {
----------------
Nit: `std::move` the iterators here as well?
================
Comment at: libcxx/include/__algorithm/ranges_ends_with.h:117
+ }
+ }
+ template <input_iterator _Iter1,
----------------
Nit: please add a blank line after this line (to separate different functions from each other).
================
Comment at: libcxx/include/__algorithm/ranges_ends_with.h:167
+ __offset);
+ } else {
+ return __ends_with_fn_impl(
----------------
Nit: please add a blank line before this line.
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.ends_with/ranges.ends_with.pass.cpp:42
+static_assert(!HasEndsWithIt<ForwardIteratorNotIncrementable>);
+static_assert(!HasEndsWithIt<int*, SentinelForNotSemiregular>);
+static_assert(!HasEndsWithIt<int*, int*, int**>); // not indirectly comparable
----------------
Nit: I think we need one successful test case for each number of template arguments, so here we need to add:
```
static_assert(!HasEndsWithIt<int*, int*>);
```
(this is to "test the test", so to speak)
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.ends_with/ranges.ends_with.pass.cpp:51
+template <class Range1, class Range2 = UncheckedRange<int*>>
+concept HasEndsWithR = requires(Range1 range1, Range2 range2) { std::ranges::ends_with(range1, range2); };
+
----------------
Nit: `Range1` and `Range2` should be forwarding references, and in the body of the concept, `range1` and `range2` should be forwarded. That way, it would be more similar to how the function is actually invoked.
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.ends_with/ranges.ends_with.pass.cpp:58
+static_assert(!HasEndsWithR<ForwardRangeNotSentinelEqualityComparableWith>);
+static_assert(!HasEndsWithR<UncheckedRange<int*>, UncheckedRange<int**>>); // not indirectly comparable
+static_assert(!HasEndsWithR<UncheckedRange<int*>, ForwardRangeNotDerivedFrom>);
----------------
Here we also need to have a successful test case for two template arguments.
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.ends_with/ranges.ends_with.pass.cpp:59
+static_assert(!HasEndsWithR<UncheckedRange<int*>, UncheckedRange<int**>>); // not indirectly comparable
+static_assert(!HasEndsWithR<UncheckedRange<int*>, ForwardRangeNotDerivedFrom>);
+static_assert(!HasEndsWithR<UncheckedRange<int*>, ForwardRangeNotIncrementable>);
----------------
Optional: IMO it's not necessary to test out every single not-forward-range we have, just a couple is enough (e.g. I would leave just `ForwardRangeNotDerivedFrom` and `ForwardRangeNotSentinelSemiregular`).
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.ends_with/ranges.ends_with.pass.cpp:234
+ { // check that the projections are used
+ int a[] = {1, 3, 15, 1, 2, 1};
+ int p[] = {2, 1, 2};
----------------
Nit: formatting needs fixing.
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.ends_with/ranges.ends_with.pass.cpp:255
+ // This is to test (forward_iterator<_Iter1> || sized_sentinel_for<_Sent1, _Iter1>) condition.
+ types::for_each(types::cpp20_input_iterator_list<int*>{}, []<class Iter2>() {
+ types::for_each(types::cpp20_input_iterator_list<int*>{}, []<class Iter1>() {
----------------
I think this should be `random_access_iterator` list, otherwise none of the iterator types in the list are a forward iterator so the `if constexpr (std::forward_iterator...)` below will never be taken.
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.ends_with/ranges.ends_with.pass.cpp:274
+
+ { // benmark test to check that random access optimization
+ std::__1::vector<int> a = {100, 32};
----------------
ldionne wrote:
> Instead, we should add a benchmark under `<monorepo>/libcxx/benchmarks` and post the results of that benchmark here in this review. I know this isn't an automated test, but the test as-is risks being flaky. Let's leave the benchmarks in `libcxx/benchmarks` and we can figure out a way to track performance through time (and potentially compare performance of different operations?) more generally.
Nit: `s/benmark/benchmark/` (here and below).
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.ends_with/ranges.ends_with.pass.cpp:274
+
+ { // benmark test to check that random access optimization
+ std::__1::vector<int> a = {100, 32};
----------------
var-const wrote:
> ldionne wrote:
> > Instead, we should add a benchmark under `<monorepo>/libcxx/benchmarks` and post the results of that benchmark here in this review. I know this isn't an automated test, but the test as-is risks being flaky. Let's leave the benchmarks in `libcxx/benchmarks` and we can figure out a way to track performance through time (and potentially compare performance of different operations?) more generally.
> Nit: `s/benmark/benchmark/` (here and below).
+1, benchmarks should be under the `benchmarks` folder and use the infrastructure we're currently using (that is, Google Benchmark).
Also, we shouldn't be benchmarking the implementation details like this, partially because it's easy for the benchmark and the actual implementation to get out of sync. Rather, you can use the same input vectors for different test cases but wrap their iterators into our helper iterators, then call `ends_with` using the wrappers:
```
std::vector<int> a = /*some initializer*/;
std::vector<int> p = /*some initializer*/;
{
std::random_access_iterator<int*> begin1(a.begin());
std::random_access_iterator<int*> end1(a.end());
std::random_access_iterator<int*> begin2(p.begin());
std::random_access_iterator<int*> end2(p.end());
std::ranges::ends_with(begin1, end1, begin2, end2);
// ...
}
{
std::bidirectional_iterator<int*> begin1(a.begin());
std::bidirectional_iterator <int*> end1(a.end());
std::bidirectional_iterator <int*> begin2(p.begin());
std::bidirectional_iterator <int*> end2(p.end());
std::ranges::ends_with(begin1, end1, begin2, end2);
// ...
}
```
You can even test the sized optimization using `sized_sentinel`. Like Louis mentioned above, we unfortunately don't have a way to test for performance regressions currently, and we shouldn't try to solve that problem in this patch. Rather, it would be great if you could run the new benchmarks locally, make sure the numbers are as expected (i.e., random access iterators are faster than bidirectional iterators, etc.), then post the results in this review.
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.ends_with/ranges.ends_with.pass.cpp:275
+ { // benmark test to check that random access optimization
+ std::__1::vector<int> a = {100, 32};
+ std::__1::vector<int> p = {32};
----------------
This should be just `std::vector`.
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.ends_with/ranges.ends_with.pass.cpp:277
+ std::__1::vector<int> p = {32};
+ for(int i = 0; i < 2147483640; i++) {
+ p.push_back(i);
----------------
Nit: formatting (should be a space after `for`, i.e., `for (`).
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.ends_with/ranges.ends_with.pass.cpp:283
+ { // test bidirectional algorithm as a benmark
+ auto __rbegin1 = std::make_reverse_iterator(a.end());
+ auto __rend1 = std::make_reverse_iterator(a.begin());
----------------
Names used in tests don't have to be uglified with leading `__` underscores (in fact, they shouldn't be since these are technically reserved names for the library. Even though the test is testing the library, the test itself isn't a part of the library).
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.ends_with/ranges.ends_with.pass.cpp:288
+ bool ret = std::ranges::starts_with(__rbegin1, __rend1, __rbegin2,
+ __rend2, {}, {}, {});
+ assert(ret);
----------------
Since these are the default values, I think they should be omitted (the predicate and the projections, that is).
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