This is an archive of the discontinued LLVM Phabricator instance.

[libcxx][ranges] adds `ranges::range`, `ranges::common_range`, and range aliases
ClosedPublic

Authored by cjdb on Apr 11 2021, 10:44 AM.

Details

Summary
  • std::ranges::range
  • std::ranges::sentinel_t
  • std::ranges::range_difference_t
  • std::ranges::range_value_t
  • std::ranges::range_reference_t
  • std::ranges::range_rvalue_reference_t
  • std::ranges::common_range

range_size_t depends on sized_range and will be added alongside it.

Implements parts of:

  • P0896R4 The One Ranges Proposal`

Depends on D100255.

Diff Detail

Event Timeline

cjdb created this revision.Apr 11 2021, 10:44 AM
cjdb requested review of this revision.Apr 11 2021, 10:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 11 2021, 10:44 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
cjdb updated this revision to Diff 336703.Apr 11 2021, 2:42 PM

rebases to activate CI

cjdb updated this revision to Diff 337813.Apr 15 2021, 9:56 AM

rebases to activate CI

miscco added inline comments.Apr 15 2021, 11:53 AM
libcxx/include/ranges
27–30

Is there some duplication here?

cjdb updated this revision to Diff 339298.Apr 21 2021, 10:12 AM

uploading so @EricWF can take a look

cjdb updated this revision to Diff 339408.Apr 21 2021, 3:24 PM

rebases to activate CI

In general looks good. Some minor remarks, but would like to see it pass the CI.

libcxx/include/CMakeLists.txt
137

This seems unintentional.

libcxx/include/__ranges/concepts.h
30

Is the synopsis in the <ranges> header?

42

Please _Uglify. Note more to _Uglify below.

44

Shouldn't

template<sized_­range R>
    using range_size_t = decltype(ranges::size(declval<R&>()));

be included here? Or is it somewhere else? If it's somewhere else can you add a comment like you did for iterator_t?

libcxx/test/std/containers/unord/unord.multimap/range_concept_conformance.compile.pass.cpp
19 ↗(On Diff #339408)

Why no non-const version?

libcxx/test/std/strings/basic.string/range_concept_conformance.compile.pass.cpp
22 ↗(On Diff #339408)

Can you also test std::basic_string with other character types, wchar_t, char8_t, char16_t, and char32_t?

cjdb updated this revision to Diff 340318.Apr 24 2021, 5:43 PM
cjdb marked 5 inline comments as done.
cjdb edited the summary of this revision. (Show Details)
  • adds std::ranges::common_range
  • adds conformance tests
  • adds missing content to synopsis
  • _Uglifies names
libcxx/include/CMakeLists.txt
137

Oops.

libcxx/include/__ranges/concepts.h
44

Please see the commit message.

libcxx/include/ranges
27–30

Where?

libcxx/test/std/strings/basic.string/range_concept_conformance.compile.pass.cpp
22 ↗(On Diff #339408)

I'm not convinced those will yield anything useful since they're aliases, not specialisations.

cjdb updated this revision to Diff 340329.Apr 24 2021, 10:19 PM
cjdb retitled this revision from [libcxx] adds concept `range` and range aliases to [libcxx][ranges] adds `ranges::range`, `ranges::common_range`, and range aliases.

fixes Windows build failure

Mordante added inline comments.Apr 25 2021, 4:47 AM
libcxx/include/__ranges/concepts.h
44

I see, missed that during the original review.

libcxx/test/std/containers/associative/map/range_concept_conformance.compile.pass.cpp
12 ↗(On Diff #340329)

Not too fond of this, see my post-commit comments for D101205.

ldionne accepted this revision.Apr 27 2021, 9:59 AM

Overall LGTM, but I think it makes more sense for iterator_t to live in __ranges/concepts.h if possible.

libcxx/include/__ranges/concepts.h
39

That's kinda weird, would it be possible to define it here instead?

This revision is now accepted and ready to land.Apr 27 2021, 9:59 AM
cjdb added inline comments.Apr 27 2021, 10:02 AM
libcxx/include/__ranges/concepts.h
39

Your options are:

  • it lives here exclusively, __ranges/access.h (__ranges/end.h in current D100255) needs to use decltype(ranges::begin(declval<_Rp&>())) wherever it currently uses iterator_t
  • it lives in __ranges/access.h exclusively, comment remains
  • it lives in both
libcxx/include/__ranges/concepts.h
39

FWIW, I'd look at the definitions on cppreference:

  • iterator_t is the type of ranges::begin, so it belongs next to begin
  • sentinel_t is the type of ranges::end, so it belongs next to end
  • range_size_tis the type of ranges::size, so it belongs next to size

The other four typedefs probably might as well go into <ranges> until we have a reason to move them elsewhere.

libcxx/include/ranges
50–51

This is listed twice.

cjdb added inline comments.Apr 27 2021, 2:24 PM
libcxx/include/__ranges/concepts.h
39

FWIW, I'd look at the definitions on cppreference:

Having written a fair chunk of the ranges section of cppreference, I can confidently say it's designed for consumers of C++, not for implementers, so different groupings make sense.

  • iterator_t is the type of ranges::begin, so it belongs next to begin
  • sentinel_t is the type of ranges::end, so it belongs next to end
  • range_size_tis the type of ranges::size, so it belongs next to size

This is a good suggestion (even though cppreference doesn't group them like this), but I'd prefer to group them as per the standard's synopsis.

The other four typedefs probably might as well go into <ranges> until we have a reason to move them elsewhere.

That would make using them in <ranges/*> very awkward.

ldionne added inline comments.Apr 28 2021, 7:46 AM
libcxx/include/__ranges/concepts.h
39

Hmm, I think I buy Arthur's argument. Either do that or leave as-is, non blocking as far as I'm concerned. Thanks for explaining why I can't just have what I asked for in the first place, though.

cjdb added a subscriber: cor3ntin.Apr 29 2021, 10:37 AM
cjdb updated this revision to Diff 341729.Apr 29 2021, 6:29 PM

rebases to activate CI