[libcxx-commits] [PATCH] D150831: [libc++] Implement ranges::ends_with

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.ends_with/ranges.ends_with.pass.cpp:52
+template <class Range1, class Range2 = UncheckedRange<int*>>
+concept HasEndsWithR = requires(Range1&& range1, Range2&& range2) { std::ranges::ends_with(range1, range2); };
+
----------------
Nit: you would also need to call `std::forward<Range1>(range1)` here (same for the other argument).


================
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:
> 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.
Sorry, rereading this comment, I mean `random_access_iterator` and `bidirectional_iterator` from our `test_iterators.h` header. Please ignore the `std::` qualification, I copy-pasted it accidentally. So basically
```
std::vector<int> a = /*some initializer*/;
std::vector<int> p = /*some initializer*/;

{
  random_access_iterator<int*> begin1(a.begin());
  random_access_iterator<int*> end1(a.end());
  random_access_iterator<int*> begin2(p.begin());
  random_access_iterator<int*> end2(p.end());
  std::ranges::ends_with(begin1, end1, begin2, end2);
  // ...
}
{
  bidirectional_iterator<int*> begin1(a.begin());
  bidirectional_iterator<int*> end1(a.end());
  bidirectional_iterator<int*> begin2(p.begin());
  bidirectional_iterator<int*> end2(p.end());
  std::ranges::ends_with(begin1, end1, begin2, end2);
  // ...
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150831