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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

In general looks good, but several minor issues.



================
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);
----------------
This differs from the wording `invoke(proj, *i) == value for the overloads with a parameter proj but no parameter pred;`
Does your code generate the same assembly as the wording in the Standard or does it generate extra overhead?


================
Comment at: libcxx/include/algorithm:91
+    constexpr iter_difference_t<I>
+      ranges::count(I first, S last, const T& value, Proj proj = {});               // since C++20
+
----------------
This is already in the namespace ranges and matches the other functions.
(The same for the other new functions.)


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.count/ranges.count.pass.cpp:34
+template <class It, class Sent = It>
+concept HasFindIt = requires(It it, Sent sent) { std::ranges::count(it, sent, *it); };
+static_assert(HasFindIt<int*>);
----------------
`HasFindIt` smells like a copy-paste, please update the name.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.count/ranges.count.pass.cpp:84
+    {
+      int a[] = {1, 2, 3, 4};
+      auto ret = std::ranges::count(a, a + 4, a + 3, [](int& i) { return &i; });
----------------
To avoid duplication this line could be placed in its parent block.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.count/ranges.count.pass.cpp:137
+    {
+      std::array<int ,0> a = {};
+      auto ret = std::ranges::count(a.begin(), a.end(), 1);
----------------



================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.count/ranges.count_if.pass.cpp:48
+template <class Pred>
+concept HasFindIfPred = requires(int* it, Pred pred) {std::ranges::count_if(it, it, pred); };
+
----------------
Another copy-paste.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.count/ranges.count_if.pass.cpp:68
+    std::same_as<std::ptrdiff_t> auto ret =
+      std::ranges::count_if(It(a), Sent(It(a + 4)), [](int x) mutable { return x == 4; });
+    assert(ret == 1);
----------------
Why the `mutable`? I see it used at multiple places.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.count/ranges.count_if.pass.cpp:126
+  {
+    // check that 0 is returned with no match
+    {
----------------
This comment is wrong.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.count/ranges.count_if.pass.cpp:168
+    {
+      std::array<int ,0> a = {};
+      auto ret = std::ranges::count_if(a.begin(), a.end(), [](int) { return true; });
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121523