[libcxx-commits] [PATCH] D121248: [libc++][ranges] Implement ranges::find{, _if, _if_not}

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
Comment at: libcxx/include/__algorithm/ranges_find.h:48
+    auto __pred = [&](auto&& __e) { return __e == __value; };
+    return ranges::__find_if_impl(ranges::begin(__r), ranges::end(__r), __pred, __proj);
+  }
----------------
Quuxplusone wrote:
> philnik wrote:
> > var-const wrote:
> > > Question: does `__r` need to be forwarded?
> > Definitely not the first one, and I also don't think the second one. We still need our range for the iterators to be valid, so conceptually it doesn't make sense to forward them (and makes probably no practical difference).
> Right, you never forward ranges. The Ranges idiom is "Take by forwarding reference, pass by lvalue." Because Ranges uses rvalueness to signify //short lifetime//, rather than //pilferability//; see
> https://quuxplusone.github.io/blog/2022/02/02/look-what-they-need/#if-you-see-code-using-std-forwar
> (See also Tristan Brindle's response https://tristanbrindle.com/posts/ranges-and-forwarding-references , which is kind of a prequel to that sentence of my post: it explains that Ranges uses constness to signify //nothing// rather than //not-gonna-modify-it-ness//, which explains why non-modifying algorithms can't just use `const T&` and have to use `T&&` in the first place.)
Thanks a lot for the explanation and the links.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find.pass.cpp:33
+  {
+    int a[] = {1, 2, 3, 4};
+    std::same_as<It> auto ret = std::ranges::find(It(a), Sent(It(a + 4)), 4);
----------------
philnik wrote:
> var-const wrote:
> > Consider adding the following test cases (in all 3 files):
> > - empty range;
> > - no element satisfies the predicate;
> > - all elements satisfy the predicate (also check that it is the first equivalent element that is returned).
> no elements satisfying the predicate is already checked with `// check that std::invoke is used` and all elements satisfy the predicate should effectively be checked by `// count invocations of the projection`, since that checks that there are exactly two projections. Or do you want to check for something more that I'm not seeing?
I think each test case should have a single focus -- it makes it easier to read and better protects against future changes. If someone were to remove a test case that says `// No element satisfies the predicate`, it should be pretty obvious that  test coverage is reduced. On the other hand, if someone were to modify `// check that std::invoke is used` so that it no longer covers the "no match" case, it's not immediately clear that this case may no longer be checked at all (and reading through every single test case to see if it's covered elsewhere is impractical).


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find.pass.cpp:33
+  {
+    int a[] = {1, 2, 3, 4};
+    std::same_as<It> auto ret = std::ranges::find(It(a), Sent(It(a + 4)), 4);
----------------
var-const wrote:
> philnik wrote:
> > var-const wrote:
> > > Consider adding the following test cases (in all 3 files):
> > > - empty range;
> > > - no element satisfies the predicate;
> > > - all elements satisfy the predicate (also check that it is the first equivalent element that is returned).
> > no elements satisfying the predicate is already checked with `// check that std::invoke is used` and all elements satisfy the predicate should effectively be checked by `// count invocations of the projection`, since that checks that there are exactly two projections. Or do you want to check for something more that I'm not seeing?
> I think each test case should have a single focus -- it makes it easier to read and better protects against future changes. If someone were to remove a test case that says `// No element satisfies the predicate`, it should be pretty obvious that  test coverage is reduced. On the other hand, if someone were to modify `// check that std::invoke is used` so that it no longer covers the "no match" case, it's not immediately clear that this case may no longer be checked at all (and reading through every single test case to see if it's covered elsewhere is impractical).
(The new tests apply to all 3 algorithms. AFAICT, the new empty range test still needs to be added to `find_if.pass.cpp` and `find_if_not.pass.cpp`)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121248