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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
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()); }
+
----------------
Quuxplusone wrote:
> 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.
I tried your suggestion but it merely shifts the repetition from one place to another (unless I misunderstood what you were thinking of). Whatever I do, I end up with something that's not much better than the patch as-is, cause the implementation of `__unwrap_subrange` needs to contain the duplication. Were you thinking about something else?

I did remove all those `std::forward` though, since they don't add anything and just double up the length of the line.


================
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>;
----------------
Quuxplusone wrote:
> 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.
Changed to `bidirectional_iterator<int*>` instead of `ranges::iterator_t`, but I'll keep using `BidirRange` since it's more minimal than `std::list`.


================
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);
----------------
Quuxplusone wrote:
> 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.
> 
You're right, but it seems like we haven't implemented `ranges::rbegin` and `ranges::rend` yet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110426