This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Implement ranges::view
ClosedPublic

Authored by ldionne on Apr 29 2021, 10:02 AM.

Details

Reviewers
zoecarver
cjdb
Group Reviewers
Restricted Project
Commits
rG2021d272ad6c: [libc++] Implement ranges::view

Diff Detail

Event Timeline

ldionne created this revision.Apr 29 2021, 10:02 AM
ldionne requested review of this revision.Apr 29 2021, 10:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 29 2021, 10:02 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne added a subscriber: cjdb.Apr 29 2021, 10:05 AM
ldionne added inline comments.
libcxx/test/std/ranges/range.view/view.compile.pass.cpp
28–31

@cjdb I found it very surprising that I had to provide both those overloads for the range concept to be satisfied. Is that really intended, or is that a bug in the patch that adds range (which is under review)?

cjdb added a subscriber: tcanens.Apr 29 2021, 10:10 AM
cjdb added inline comments.
libcxx/test/std/ranges/range.view/view.compile.pass.cpp
28–31

This is a very annoying artefact of the poison pills. @tcanens, what's the likelihood we can get LWG to review their existence?

zoecarver accepted this revision as: zoecarver.Apr 29 2021, 10:37 AM
zoecarver added a subscriber: zoecarver.

Nice! Looks good sans the nits.

libcxx/docs/OneRangesProposalStatus.csv
43

Is there anything else here? I think you can mark it as complete.

libcxx/include/ranges
68–69

We need to keep this.

libcxx/test/std/ranges/range.view/view.compile.pass.cpp
37

Nit: spacing (and elsewhere).

zoecarver added inline comments.Apr 29 2021, 10:39 AM
libcxx/test/std/ranges/range.view/enable_view.compile.pass.cpp
24

What if you named these more like the types in the other test file? So this would be Empty, next would be PrivateViewBase, then EnableViewFalse, then DerivesViewBase, then EnableViewTrue (or something similar).

tcanens added inline comments.Apr 29 2021, 11:06 AM
libcxx/test/std/ranges/range.view/view.compile.pass.cpp
28–31

Would definitely take a paper and implementation experience.

ldionne updated this revision to Diff 341977.Apr 30 2021, 10:46 AM
ldionne marked 6 inline comments as done.

Apply review comments

🚢 it.

libcxx/include/ranges
68–69

Never mind I see we're including this twice 🤦‍♂️.

ldionne added inline comments.Apr 30 2021, 12:05 PM
libcxx/test/std/ranges/range.view/view.compile.pass.cpp
37

So the original reason to indent it that way was to make the thing we're actually trying to test stand out. But I guess I'll remove it :-).

cjdb requested changes to this revision.Apr 30 2021, 12:10 PM

LGTM overall, tests are very elegant! Would you mind adding checks that our existing ranges aren't views to the conformance tests please?

This revision now requires changes to proceed.Apr 30 2021, 12:10 PM
ldionne updated this revision to Diff 342500.May 3 2021, 12:12 PM
ldionne marked an inline comment as done.

Address review comments.

cjdb accepted this revision.May 3 2021, 12:26 PM

LGTM, pending a subsumption test.

ldionne updated this revision to Diff 342516.May 3 2021, 12:49 PM

Add subsumption test.

ldionne accepted this revision as: Restricted Project.May 3 2021, 12:54 PM

Will ship once CI passes.

This revision is now accepted and ready to land.May 3 2021, 12:54 PM
ldionne updated this revision to Diff 342742.May 4 2021, 8:04 AM

Rebase onto main

This revision was landed with ongoing or failed builds.May 4 2021, 8:06 AM
This revision was automatically updated to reflect the committed changes.