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

[Tim Song via Phabricator via libcxx-commits]

tcanens 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:
> ldionne wrote:
> > 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.
> > I tried your suggestion but it merely shifts the repetition from one place to another (unless I misunderstood what you were thinking of).
> 
> I'm no longer sure myself, so, okay. :)
> Re removing the `forward`s: I think that's OK, but it would be good to check with @tcanens or someone. My rule for coding with Ranges is "Always pass by forwarding reference, never `forward`," because forwarding can only ever turn lvalues back into rvalues, and Ranges hates rvalues. However, is it conceivable that the user could supply some evil kind of `_Range` where `r.begin()` and `std::move(r).begin()` do different things? or is that forbidden by the concept?
`ranges::begin` only ever calls `begin` on lvalues nowadays.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110426