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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__ranges/counted.h:60
+      auto __sent = __it + static_cast<iter_difference_t<_Iter>>(__c);
+      return ranges::subrange(_VSTD::forward<_Iter>(__it), _VSTD::forward<_Iter>(__sent));
+    }
----------------
tcanens wrote:
> For `__sent` shouldn't it be a move?
Yes, and naming-bikeshed: can we call it `__last` instead of `__sent`? `__sent` looks like it should be a variable of type `_Sent`/`_Sp`, not an `_Iter`.
IOW, seeing `forward<_Iter>(__sent)` was the red flag here.

Also on line 56, `noexcept(ranges::subrange(_VSTD::forward<_Iter>(__it), _VSTD::__decay_copy(__it)))`
or something like that. Gross. Does the standard say "expression-equivalent" so we have to get this right? or is leaving it as non-noexcept an option?


================
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:
> 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.
@ldionne: The situation here is easy, unless I'm mistaken: `std::views::counted((int*)iter, 8)` returns a `span<int>`, which is implicitly convertible to `span<const int>`, so that's what happens.
I agree that there should be a `static_assert` on the actual decltype, because actually tons of types are implicitly convertible to `span<const int>`. Including `subrange<int*,int*>`, for example, which would totally not be the conforming type to return here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106923