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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/__algorithm/ranges_count_if.h:38
+  for (; __first != __last; ++__first) {
+    if (std::invoke(__pred, std::invoke(__proj, *__first)))
+      ++__counter;
----------------
var-const wrote:
> 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?
> 
Yes. `if`s can use explicit conversions without the type being explicitly cast to `bool`. (https://godbolt.org/z/xr8nEb6xG)


================
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 {
----------------
var-const wrote:
> 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?
There are constraints through `input_iterator` -> `input_or_output_iterator` -> `weakly_incrementable`. Specifically `__signed_integer_like`.


================
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*>);
+
----------------
var-const wrote:
> I think there are a couple more SFINAE cases to test:
> - `projected<I, P>` is invalid;
> - `iter/range_difference_t` is invalid.
`{iter, range}_difference_t` is never invalid if `input_{iterator, range}` is valid. With `projected` i guess you meant `indirect_binary_predicate`? There I don't think I can do much more than `NotEqualityComparable`.


================
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);
----------------
var-const wrote:
> Question: why not use `a.begin()` and `a.end()`?
`a.begin()` and `a.end()` aren't portably `int*`, but they have to be `int*` for the casts to work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121523