[libcxx-commits] [PATCH] D121523: [libc++][ranges] Implement ranges::count{, _if}

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

LGTM with my and Costa's comments. Thanks!



================
Comment at: libcxx/include/__algorithm/ranges_count.h:38
+  iter_difference_t<_Ip> operator()(_Ip __first, _Sp __last, const _Tp& __value, _Proj __proj = {}) const {
+    auto __pred = [&](const auto& __e) { return __e == __value; };
+    return ranges::__count_if_impl(std::move(__first), std::move(__last), __pred, __proj);
----------------
ldionne wrote:
> @tcanens 's comment on `ranges::find` probably applies here too -- this should be `-> bool` IIUC (please add test, too).
> 
> Also, we should use `auto&& __e`.
My comment about `-> bool` can be ignored since this code is correct as-is. However, can we add tests like we did in D122011?


================
Comment at: libcxx/test/libcxx/diagnostics/detail.headers/algorithm/ranges_count_if.module.verify.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
Please rebase onto `main` with the new way we generate those tests.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.count/ranges.count.pass.cpp:204-212
+    // check that an immobile type works
+    struct Immobile {
+      Immobile(const Immobile&) = delete;
+      Immobile(Immobile&&) = delete;
+      constexpr Immobile(int i_) : i(i_) {}
+      int i;
+
----------------
We usually say "NonMovable" instead of "Immobile". If you change it, it applies to the other test as well. Non-blocking, but it would be more idiomatic IMO.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.count/ranges.count.pass.cpp:59-69
+  {
+    int a[] = {1, 2, 3, 4};
+    std::same_as<std::ptrdiff_t> auto ret = std::ranges::count(It(a), Sent(It(a + 4)), 4);
+    assert(ret == 1);
+  }
+  {
+    int a[] = {1, 2, 3, 4};
----------------
philnik wrote:
> ldionne wrote:
> > Here, you should test with a 0-sized range and 1-sized range too. And for each range size (when that makes sense), you can vary the position of the element you're finding. Right now, you're always finding the last element in the range -- that's not exhaustive enough. You can also check for an element which is not in the range.
> > 
> > Then, you can remove a couple of tests below that are only run on `int[]`, like the one that says `// check that 0 is returned with no match`.
> > 
> > This comment applies to the other algorithms patches I haven't looked at yet -- we need to be more exhaustive when checking algorithms, especially since it's so easy to do.
> I'm not sure what you are asking for. Do you want that the result changes for every iterator type? Or just have more invocations for each of them?
I think you can disregard this comment. I am not sure what I had in mind anymore, but I seem to have written it thinking that we were testing `ranges::find`, which is obviously not the case. I looked at the tests again and I don't think we are missing relevant coverage.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121523