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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik 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);
+  }
----------------
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).


================
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:
> 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121248