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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a subscriber: tcanens.
ldionne added a comment.
This revision now requires changes to proceed.

Looks pretty good, with some comments especially about exhaustiveness in the tests.



================
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);
----------------
@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`.


================
Comment at: libcxx/include/__algorithm/ranges_count_if.h:32
+namespace ranges {
+template <class _Ip, class _Sp, class _Proj, class _Pred>
+_LIBCPP_HIDE_FROM_ABI constexpr
----------------
FWIW, I'm not a huge fan of extremely terse names like this. I'd find `_Iterator` and `_Sentinel` clearer. This is non-blocking, but that would be easier to read, and it's not like we gain anything by using fewer characters. This applies throughout the patch.


================
Comment at: libcxx/include/__algorithm/ranges_count_if.h:34
+_LIBCPP_HIDE_FROM_ABI constexpr
+auto __count_if_impl(_Ip __first, _Sp __last, _Pred& __pred, _Proj& __proj) {
+  iter_difference_t<_Ip> __counter(0);
----------------



================
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};
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121523