Details
- Reviewers
ldionne cjdb - Group Reviewers
Restricted Project - Commits
- rGf9e58f35e905: [libcxx][ranges] Add `views::counted` CPO.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| libcxx/include/__ranges/counted.h | ||
|---|---|---|
| 40 | Throughout, could you use _Iter instead of _Tp and perhaps __it instead of __t? I think it conveys the meaning a bit better. | |
| libcxx/test/std/ranges/range.adaptors/range.counted/counted.pass.cpp | ||
| 2 | We need to test that views::counted is a CPO, see http://eel.is/c++draft/customization.point.object. | |
| 4 | Can you add a test that views::counted(iter, diff) is SFINAEd out when diff is not convertible to the iter_difference_t of iter, per http://eel.is/c++draft/range.adaptors#range.counted-2? | |
| 30 | I'm not immediately seeing why this is std::span<const int> and not std::span<int>, can you explain? | |
| 74 | Can you also add a test with a forward_iterator? | |
| libcxx/include/__ranges/counted.h | ||
|---|---|---|
| 43 | ||
| libcxx/test/std/ranges/range.adaptors/range.counted/counted.pass.cpp | ||
| 2 | I think that's fine. | |
| 30 | 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. | |
| 52 | Can you also add tests for contiguous_iterator<int const*>? And for all the other iterator categories below. | |
Based on the CI failures, you've got some missing #include. Ship it once the CI is happy!
| libcxx/include/__ranges/counted.h | ||
|---|---|---|
| 43 | There are things that are implicitly-but-not-explicitly convertible to iter_difference_t<_Iter>, in which case this compiles but the wording calls for rejection. | |
| 48 | Why the static_cast to the same type? The forward is also unnecessary - the to_address overload for non-pointers should take by const& anyway. | |
| 61 | For __sent shouldn't it be a move? | |
| libcxx/include/__ranges/counted.h | ||
|---|---|---|
| 61 | 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. Also on line 56, noexcept(ranges::subrange(_VSTD::forward<_Iter>(__it), _VSTD::__decay_copy(__it))) | |
| libcxx/test/std/ranges/range.adaptors/range.counted/counted.pass.cpp | ||
| 30 | @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. | |
Apply review comments. Fix build.
| libcxx/include/__ranges/counted.h | ||
|---|---|---|
| 48 | Now that the second parameter is a template type parameter type I'm going to keep the static_cast. | |
Throughout, could you use _Iter instead of _Tp and perhaps __it instead of __t? I think it conveys the meaning a bit better.