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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante accepted this revision as: Mordante.
Mordante added a comment.

It looks like the CI failures aren't related to this change. LGTM, modulo one small issue.



================
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);
----------------
philnik wrote:
> Mordante wrote:
> > 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?
> What exactly do you think differs between my implementation and the standard? (https://godbolt.org/z/GvGvT7fxT) At least my `ranges::count` and `std::count` produce for this example the exact same assembly at `-O1`.
The indirection using the lambda is the difference. For `-O0`, the version with the lambda generates a lot more code for this version. Starting at `-O1` both versions generate the same output.
But I don't feel too strongly about slightly worse codegen at `-O0`.


================
Comment at: libcxx/include/__algorithm/ranges_count_if.h:27
+
+#if !defined(_LIBCPP_HAS_NO_CONCEPTS) && !defined(_LIBCPP_HAS_NO_INCOMPLETE_RANGES)
+
----------------
Sorry I missed this in the previous review, but `_LIBCPP_HAS_NO_CONCEPTS` is no longer required.


================
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; });
----------------
philnik wrote:
> Mordante wrote:
> > To avoid duplication this line could be placed in its parent block.
> Arthur asked me to duplicate it in other tests to have the definition near the usage.
For these small tests I would prefer to avoid duplication. However since Arthur asked you to do it this way, let's keep it as-is.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.count/ranges.count_if.pass.cpp:126
+  {
+    // check that 0 is returned with no match
+    {
----------------
philnik wrote:
> Mordante wrote:
> > This comment is wrong.
> What's wrong with the comment? It checks that 0 is returned when there is no match?
Sorry I misread, please ignore my previous remark.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121523