[libcxx-commits] [PATCH] D106923: [libcxx][ranges] Add `views::counted` CPO.

[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/__ranges/counted.h:42
+    _LIBCPP_HIDE_FROM_ABI
+    constexpr auto operator()(_Iter&& __t, iter_difference_t<_Iter> __c) const
+      noexcept(noexcept(
----------------



================
Comment at: libcxx/test/std/ranges/range.adaptors/range.counted/counted.pass.cpp:51
+    {
+      const contiguous_iterator<int*> iter(buffer);
+      std::span<int> s = std::views::counted(iter, 8);
----------------
Can you also add tests for `contiguous_iterator<int const*>`? And for all the other iterator categories below.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.counted/counted.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
zoecarver wrote:
> ldionne wrote:
> > We need to test that `views::counted` is a CPO, see http://eel.is/c++draft/customization.point.object.
> Added a test for semiregular. Are there other tests you want? Maybe noexcept tests? 
I think that's fine.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.counted/counted.pass.cpp:29
+      contiguous_iterator<int*> iter(buffer);
+      std::span<const int> s = std::views::counted(iter, 8);
+      assert(s.size() == 8);
----------------
ldionne wrote:
> I'm not immediately seeing why this is `std::span<const int>` and not `std::span<int>`, can you explain?
Also, can you please `static_assert(std::is_same_v<decltype(std::views::counted(...)), what-you-expect>);`. Otherwise, we're potentially relying on conversions between span-to-const and span-to-non-const.

Let's figure out the const situation below, because it makes little sense to my understanding.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106923