Page MenuHomePhabricator

[libc++] Add the std::views::reverse range adaptor

Authored by ldionne on Sep 24 2021, 9:26 AM.


Diff Detail

Event Timeline

ldionne requested review of this revision.Sep 24 2021, 9:26 AM
ldionne created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptSep 24 2021, 9:26 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone added inline comments.

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.


Rather than the _UnwrappedSubrange parameter, maybe consider something like

-> 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.


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.


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.

ldionne updated this revision to Diff 375376.Sep 27 2021, 12:54 PM
ldionne marked 3 inline comments as done.

Address review comments and fix bug on GCC.


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.


Changed to bidirectional_iterator<int*> instead of ranges::iterator_t, but I'll keep using BidirRange since it's more minimal than std::list.


You're right, but it seems like we haven't implemented ranges::rbegin and ranges::rend yet.

Quuxplusone added inline comments.

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 forwards: 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?

tcanens added inline comments.Sep 28 2021, 9:17 PM

ranges::begin only ever calls begin on lvalues nowadays.

Mordante accepted this revision.Oct 2 2021, 7:06 AM
Mordante added a subscriber: Mordante.


This revision is now accepted and ready to land.Oct 2 2021, 7:06 AM
This revision was automatically updated to reflect the committed changes.