Page MenuHomePhabricator

[libc++] Implements ranges::enable_borrowed_range
Needs ReviewPublic

Authored by Mordante on Nov 7 2020, 1:30 AM.

Details

Reviewers
mclow.lists
EricWF
ldionne
CaseyCarter
eric_niebler
miscco
Group Reviewers
Restricted Project
Summary

This is the initial patch to implement ranges in libc++.

Implements parts of:

  • P0896R4 One Ranges Proposal
  • P1870 forwarding-range is too subtle
  • LWG3379 in several library names is misleading

Diff Detail

Event Timeline

Mordante created this revision.Nov 7 2020, 1:30 AM
Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 7 2020, 1:30 AM
Mordante requested review of this revision.Nov 7 2020, 1:30 AM

Don't forget to add the new <ranges> header to std_includes test. Otherwise no other remarks from my side.

Don't forget to add the new <ranges> header to std_includes test. Otherwise no other remarks from my side.

Thanks for the pointer. I'll update the patch shortly.

Mordante updated this revision to Diff 303635.Nov 7 2020, 5:21 AM

Added the double_include.sh.cpp test as suggested by @curdeius.
move a test from ranges/ranges to ranges/range.range.

Mordante updated this revision to Diff 303636.Nov 7 2020, 5:45 AM

Added the other steps required when adding a new header as suggested by @tschuett.

miscco added a comment.Nov 7 2020, 1:16 PM

I am wondering whether we should split the ranges header up into a smaller part that we use to include in other headers adn the rest.

Including all of ranges just for one trait seems a bit of an overkill.

We could always add it to concepts as that should be a transitive include for everything that includes range. Thoughts @ldionne?

libcxx/include/ranges
16

I am slightly surprised that concepts is not required

23

I am wondering whether it is wise to copy the synopsis as it should be rather than how it is. This might trick the user to believe all of it is supported. What are the conventions here?

libcxx/test/std/ranges/range.range/enable_borrowed_range.pass.cpp
24 ↗(On Diff #303636)

I believe you are missing coverage for all the ranges that are not borrowed, e.g. vector, list, ...

tschuett added inline comments.Nov 7 2020, 10:26 PM
libcxx/include/ranges
326

Why is the curly bracket not at the end of the line above?

Mordante marked 4 inline comments as done.Nov 8 2020, 5:59 AM

I am wondering whether we should split the ranges header up into a smaller part that we use to include in other headers adn the rest.

Including all of ranges just for one trait seems a bit of an overkill.

I agree including the full ranges header in string_view and span feels a bit overkill. But I'll leave the decisions whether or not to split the header with the libc++ maintainers.

libcxx/include/ranges
16

Agreed. For this patch I don't need concepts, but std::ranges::begin() will require them.

23

Looking at other headers it does they do the same for example concepts also uses 'unspecified' and 'see below'.

326

Because that's what 'chrono' uses. However I have no objection against putting it on one line. Not sure what the libc++ coding style has to say about this.

libcxx/test/std/ranges/range.range/enable_borrowed_range.pass.cpp
24 ↗(On Diff #303636)

I'm not convinced they really need to be tested, but adding them is not much effort. So I'll add them.

Mordante updated this revision to Diff 303717.Nov 8 2020, 6:23 AM
Mordante marked 4 inline comments as done.

Adds the additional unit tests as requested by @miscco.
Found some accidental tabs in the patch replaced them with spaces.

Mordante updated this revision to Diff 304252.Nov 10 2020, 10:31 AM

Change the unit tests to compile time test, based on the review comment in D91004.

I am wondering whether we should split the ranges header up into a smaller part that we use to include in other headers adn the rest.

Including all of ranges just for one trait seems a bit of an overkill.

We could always add it to concepts as that should be a transitive include for everything that includes range. Thoughts @ldionne?

I'd be fine with splitting various parts of ranges into separate headers if that makes the implementation clearer, etc.

Mordante updated this revision to Diff 307561.Nov 25 2020, 2:48 AM
Mordante edited the summary of this revision. (Show Details)

Move the enable_borrowed_range to a helper header to avoid including <ranges> in <span> and <string_view> as suggested by @miscco.

Mordante updated this revision to Diff 309287.Dec 3 2020, 9:37 AM

Implement all new header tests as suggested by @curdeius in D92214.

Mordante updated this revision to Diff 318547.Jan 22 2021, 9:14 AM
  • Removed unimplemented part of the synopsis
  • Ran clang-format on the patch
  • Rebased to trigger CI again
Mordante updated this revision to Diff 318745.Jan 23 2021, 4:13 AM

Rebase to trigger CI. The last build failed, but so did main.

cjdb added a subscriber: cjdb.Jan 25 2021, 11:50 AM
Mordante updated this revision to Diff 321466.Thu, Feb 4, 8:58 AM

Rebased to trigger CI.