[libcxx-commits] [PATCH] D110426: [libc++] Add the std::views::reverse range adaptor

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

Quuxplusone accepted this revision as: Quuxplusone.
Quuxplusone added inline comments.


================
Comment at: libcxx/include/__ranges/reverse_view.h:129
+    template<class _Iter, subrange_kind _Kind>
+    constexpr bool __is_unsized_reverse_subrange<subrange<reverse_iterator<_Iter>, reverse_iterator<_Iter>, _Kind>> = _Kind != subrange_kind::sized;
+
----------------
s/`!= subrange_kind::sized`/`== subrange_kind::unsized`/
They're not likely to add any more than the existing two enumerator values to `subrange_kind`, but if they ever did, I wouldn't want to hazard a guess as to what their semantics would be.


================
Comment at: libcxx/include/__ranges/reverse_view.h:166
+        -> decltype(      _UnwrappedSubrange(_VSTD::forward<_Range>(__range).end().base(), _VSTD::forward<_Range>(__range).begin().base()))
+        { return          _UnwrappedSubrange(_VSTD::forward<_Range>(__range).end().base(), _VSTD::forward<_Range>(__range).begin().base()); }
+
----------------
Rather than the `_UnwrappedSubrange` parameter, maybe consider something like
```
noexcept(noexcept(__unwrap_subrange(_VSTD::forward<_Range>(__range))))
-> decltype(     (__unwrap_subrange(_VSTD::forward<_Range>(__range))))
```
with a `static constexpr auto __unwrap_subrange(auto&& __range)` defined appropriately. That wouldn't make the compiler's job any easier, but it would make these lines (155-157, 164-166) shorter and less repetitive.
Making `_UnwrappedSubrange(~~~)` sometimes come out to `void(~~~)` is a subtle trick.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.reverse/adaptor.pass.cpp:53
+  {
+    using It = std::ranges::iterator_t<BidirRange>;
+    using Subrange = std::ranges::subrange<It, It, std::ranges::subrange_kind::sized>;
----------------
Is `BidirRange`  defined in "types.h"? I'd guess that `std::ranges::iterator_t<BidirRange>` should be `bidirectional_iterator<int*>` (or something: what is its value type?), from "test_iterators.h". If it is, maybe we could just say that.
We could even just say
```
using C = std::list<int>;
C lst = {1,2,3};
auto reversed = std::ranges::subrange(lst.rbegin(), lst.rend());
auto rereversed = std::views::reverse(reversed);
ASSERT_SAME_TYPE(decltype(rereversed), std::ranges::subrange<C::iterator, C::iterator>);
```
but that might be too lax.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.reverse/adaptor.pass.cpp:60
+    BidirRange view(buf, buf + 3);
+    ReverseSubrange subrange(ReverseIt(std::ranges::end(view)), ReverseIt(std::ranges::begin(view)), /* size */3);
+    std::same_as<Subrange> auto result = std::views::reverse(subrange);
----------------
Why is this not just
```
auto subrange = std::ranges::subrange(std::ranges::rbegin(view), std::ranges::rend(view), 3);
```
I think that'd be the same thing, and easier to read IMHO.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110426