Page MenuHomePhabricator

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

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

Details

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.
libcxx/include/__ranges/reverse_view.h
129

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.

166

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.

libcxx/test/std/ranges/range.adaptors/range.reverse/adaptor.pass.cpp
54

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.

61

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.

libcxx/include/__ranges/reverse_view.h
166

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.

libcxx/test/std/ranges/range.adaptors/range.reverse/adaptor.pass.cpp
54

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

61

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

Quuxplusone added inline comments.
libcxx/include/__ranges/reverse_view.h
166

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
libcxx/include/__ranges/reverse_view.h
166

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

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

LGTM!

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.