[libcxx-commits] [PATCH] D156956: [libc++] Optimize ranges::count for __bit_iterators

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__algorithm/count.h:35
+_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 __policy_diff_t<_AlgPolicy, _Iter>
+__count_impl(_Iter __first, _Sent __last, const _Tp& __value, _Proj& __proj) {
+  __policy_diff_t<_AlgPolicy, _Iter> __r(0);
----------------
We normally name these algorithms just `__count`.


================
Comment at: libcxx/include/__algorithm/count.h:76
+_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX23 __iter_diff_t<__bit_iterator<_Cp, _IsConst> >
+__count_impl(__bit_iterator<_Cp, _IsConst> __first, __bit_iterator<_Cp, _IsConst> __last, const _Tp& __value, _Proj&) {
+  if (__value)
----------------
Can you make sure that these code paths are tested? We could have a test like `libcxx/test/std/algorithms/alg.nonmodifying/alg.count/count.vector_bool.pass.cpp` *or* `libcxx/test/std/containers/sequences/vector.bool/count.pass.cpp`, but we should have something.

I would recommend breaking this logic in a very obvious way to first see whether we have an existing test for this.


================
Comment at: libcxx/include/__algorithm/iterator_operations.h:176-177
 
+template <class _AlgPolicy, class _Iter>
+using __policy_diff_t = typename _IterOps<_AlgPolicy>::template __difference_type<_Iter>;
+
----------------
I think I would avoid introducing this, I don't think it provides that much value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156956