[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/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);
----------------
Quuxplusone wrote:
> var-const wrote:
> > 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`)
> > 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
> 
> +1. Of course one way around this would be to tag the single test case with //both// comments. But writing one-for-one test cases produces the side benefit of "more test coverage." E.g. maybe the std::invoke test case should take two lines and test //both// the "found" and "not found" codepaths; and maybe the not-found test case should also assert that in that situation the predicate is called exactly N times. (Or maybe that's just reintroducing the exact issue var-const objected to. That's fair. :)) Anyway, +1 to "one focus, one comment, one test case" from me. :)
Thanks, I must have forgotten to copy it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121248