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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const requested changes to this revision.
var-const added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__algorithm/ranges_count_if.h:38
+  for (; __first != __last; ++__first) {
+    if (std::invoke(__pred, std::invoke(__proj, *__first)))
+      ++__counter;
----------------
Question: in the Standard, there's an explicit cast to `bool`, but I presume it doesn't matter because the return type is implicitly converted to bool by virtue of being used in an `if` expression, right?



================
Comment at: libcxx/include/__algorithm/ranges_count_if.h:49
+  _LIBCPP_HIDE_FROM_ABI constexpr
+  iter_difference_t<_Iterator> operator()(_Iterator __first, _Sentinel __last,
+                                          _Predicate __pred, _Projection __proj = {}) const {
----------------
Question: hmm, there are no constraints on `iter/range_difference_t` (in the Standard), but the implementation has to make certain assumptions about the type. I wonder if it would be a good idea to have a constraint on that as well (unless this constraint comes from somewhere implicitly). What do you think?


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.count/ranges.count.pass.cpp:44
+static_assert(!HasCountIt<int*, int>);
+static_assert(!HasCountIt<int, int*>);
+
----------------
I think there are a couple more SFINAE cases to test:
- `projected<I, P>` is invalid;
- `iter/range_difference_t` is invalid.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.count/ranges.count.pass.cpp:59
+constexpr void test_iterators() {
+  {
+    {
----------------
Nit: can you please add a comment to this test case as well, even something as simple as "Normal case" or similar? It just makes skimming through the file easier.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.count/ranges.count.pass.cpp:63
+      std::same_as<std::ptrdiff_t> auto ret = std::ranges::count(It(a), Sent(It(a + 4)), 3);
+      assert(ret == 1);
+    }
----------------
Can you add:
- a test case where `ret > 1` to make sure the algorithm counts equivalent elements properly?
- a test case where all the elements are equal to the given element?


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.count/ranges.count.pass.cpp:77
+      std::array<int, 0> a = {};
+      auto ret = std::ranges::count(It(a.data()), Sent(It(a.data() + a.size())), 1);
+      assert(ret == 0);
----------------
Question: why not use `a.begin()` and `a.end()`?


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.count/ranges.count.pass.cpp:89
+  {
+    // check that a range with a single elment works
+    {
----------------
Nit: `s/elment/element/`.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.count/ranges.count.pass.cpp:120
+
+constexpr bool test() {
+  test_iterators<int*>();
----------------
Consider adding a test to ensure that a non-copyable (perhaps even non-movable) `_Type` works.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.count/ranges.count.pass.cpp:167
+    }
+  }
+
----------------
Can you also check that the return type is exactly `iter/range_difference_t` for a type that has some custom-defined difference type?


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.count/ranges.count_if.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
(A few comments from the other test file apply here as well)


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.count/ranges.count_if.pass.cpp:65
+constexpr void test_iterators() {
+  {
+    int a[] = {1, 2, 3, 4};
----------------
Hmm, quite a few test cases that are in `ranges.count.pass.cpp` seem to be missing here?


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.count/ranges.count_if.pass.cpp:179
+
+  return true;
+}
----------------
Can you also check that a predicate a) that takes its argument by lvalue; b) that has non-const `operator()` works?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121523