Page MenuHomePhabricator

[libcxx][iterator][ranges] adds `input_iterator` and `input_range`
ClosedPublic

Authored by cjdb on Sun, Apr 11, 1:58 PM.

Details

Summary

Implements parts of:

  • P0896R4 The One Ranges Proposal`

Depends on D100269.

Diff Detail

Event Timeline

cjdb requested review of this revision.Sun, Apr 11, 1:58 PM
cjdb created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptSun, Apr 11, 1:58 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
cjdb updated this revision to Diff 336704.Sun, Apr 11, 2:45 PM

updates synopsis, rebases to activate CI

cjdb updated this revision to Diff 337006.Mon, Apr 12, 5:58 PM

adds subsumption tests

cjdb updated this revision to Diff 337041.Mon, Apr 12, 9:38 PM

updates subsumption test

I like the test coverage but I believe that we should get @CaseyCarter on board to port the ranges test machinery over. It is quite extensive and rebuilding it for the fun of it seems a waste of time

miscco accepted this revision.Mon, Apr 12, 10:56 PM
cjdb added a comment.Tue, Apr 13, 11:44 AM

I like the test coverage but I believe that we should get @CaseyCarter on board to port the ranges test machinery over. It is quite extensive and rebuilding it for the fun of it seems a waste of time

The intention is for there to be a libc++ test suite and an MSVC STL test suite. It improves coverage and also lets us respect the different project test structures without being intrusive.
MSVC tests will be added once they can be, and after we get a script happening (I consider it P3 in terms of libc++; P6 when factoring in my day job's responsibilities).

cjdb updated this revision to Diff 337815.Thu, Apr 15, 9:59 AM

rebases to activate CI

miscco accepted this revision.Thu, Apr 15, 11:49 AM
cjdb updated this revision to Diff 339416.Wed, Apr 21, 3:50 PM

rebases so @zoecarver isn't blocked

This patch is not ready for another round of reviews

cjdb updated this revision to Diff 340330.Sat, Apr 24, 10:54 PM
cjdb retitled this revision from [libcxx] adds `input_iterator` and `input_range` to [libcxx][iterator][ranges] adds `input_iterator` and `input_range`.
  • replaces monolithic test with conformance tests
  • adds canonical test type cxx20_input_iterator
cjdb updated this revision to Diff 340364.Sun, Apr 25, 9:20 AM

fixes guards around cxx20_input_iterator

cjdb added inline comments.Sun, Apr 25, 10:48 AM
libcxx/test/std/containers/associative/set/range_concept_conformance.compile.pass.cpp
30

Should be range const (note to reviewers: please flag these as you see them so I don't miss any).

zoecarver accepted this revision as: zoecarver.Mon, Apr 26, 9:25 AM

LGTM.

libcxx/test/support/test_iterators.h
638

Side note: I know we sort of already started down the road of cxx20_ naming, but part of me doesn't really like this, because hopefully (and probably) this will just be "The Future of Iterators" so it would be great if we could name the other iterators "legacy" or whatever and make these new iterators seem as though they aren't the alternative. I don't know if this is feasible, or worth of the time it might cost, though.

libcxx/test/support/test_macros.h
316

😍

cjdb added inline comments.Mon, Apr 26, 9:51 AM
libcxx/test/support/test_iterators.h
638

D101242 takes care of renaming all the existing iterators. cxx20_ is to make it unambiguous for non-ranges contributors that it's different (this optimises for the reader).

Quuxplusone requested changes to this revision.Mon, Apr 26, 9:57 AM
Quuxplusone added inline comments.
libcxx/test/support/test_iterators.h
638

Actually, what you should do in test_iterators.h is simply make sure that our existing input_iterator is a std::input_iterator; that our existing forward_iterator is a std::forward_iterator; etc. If they don't conform to the C++20 concepts, then that is a bug worth fixing. (But I believe they do conform.)

If our existing test_iterators.h already provides C++20-compatible iterators (which, again, is the intention — and I recently even added a C++20 contiguous_iterator to this file!), then there's nothing to do here.

This revision now requires changes to proceed.Mon, Apr 26, 9:57 AM
cjdb added inline comments.Mon, Apr 26, 10:36 AM
libcxx/test/support/test_iterators.h
638

Actually, this is an important new test type for algorithms, iterator adaptors, and range adaptors.

ldionne requested changes to this revision.Tue, Apr 27, 10:23 AM

This looks OK to me apart from the input_iterator archetype naming.

libcxx/test/support/test_iterators.h
638

I'm talking to Chris offline about this, but what I'd want:

legacy_input_iterator or cpp17_input_iterator => archetype for the "old" InputIterator concept, which includes copyability
input_iterator => archetype for the new, C++20 std::input_iterator concept, without copyability

Since the other iterators don't change in any meaningful way, we shouldn't introduce new names for those.

ldionne accepted this revision.Tue, Apr 27, 12:24 PM

Reached offline agreement with @cjdb about using cpp20_input_iterator here. Chris, please change to that instead of cxx20 and ship it.

Other reviewers, please say it if you have a blocking objection with that direction, otherwise I think we've spent far enough energy on this name selection.

cjdb added a subscriber: cor3ntin.Thu, Apr 29, 10:37 AM
cjdb updated this revision to Diff 341735.Thu, Apr 29, 7:10 PM

renames cxx20_input_iterator to cpp20_input_iterator

cjdb updated this revision to Diff 341957.Fri, Apr 30, 10:01 AM

rebases to activate CI

zoecarver added inline comments.Fri, Apr 30, 10:11 AM
libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.input/subsumption.compile.pass.cpp
24

Same comment as D100275 wrt using _ITER_CONCEPT here.

cjdb updated this revision to Diff 342020.Fri, Apr 30, 1:19 PM

moves subsumption test to test/libcxx

cjdb updated this revision to Diff 342022.Fri, Apr 30, 1:26 PM

removes zero-width code point from comment

This revision was not accepted when it landed; it landed in state Needs Review.Fri, Apr 30, 3:49 PM
This revision was automatically updated to reflect the committed changes.