This is an archive of the discontinued LLVM Phabricator instance.

[libcxx][iterator][ranges] adds `forward_iterator` and `forward_range`
ClosedPublic

Authored by cjdb on Apr 11 2021, 3:12 PM.

Details

Summary

Implements parts of:

  • P0896R4 The One Ranges Proposal`

Depends on D100271.

Diff Detail

Event Timeline

cjdb requested review of this revision.Apr 11 2021, 3:12 PM
cjdb created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptApr 11 2021, 3:12 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
cjdb updated this revision to Diff 337043.Apr 12 2021, 10:00 PM

updates subsumption test

miscco accepted this revision.Apr 12 2021, 11:00 PM
miscco added inline comments.
libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.forward/forward_iterator.compile.pass.cpp
157

Feel free to disregard: That line gets drowned a bit. Should we add a banner above it?

159

I do not really like that name. We should say *why* it is not a input iterator without checking all the methods.

I think here it is not_eq_comparable_iterator. Ditto my wish for porting the iterator test machinery

cjdb added inline comments.Apr 13 2021, 2:21 PM
libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.forward/forward_iterator.compile.pass.cpp
157

Nice observation. I'll move this test (and the one below) above the namespace standard_types the next time I visit this commit (too busy to rebase this instant).

159

I do not really like that name. We should say *why* it is not a input iterator without checking all the methods.

The salient feature here isn't why the type is not an input iterator: only that it isn't an input iterator (since all forward iterators are input iterators, a non-input iterator cannot be a forward iterator). Either way, I consider it important to assert that input_iterator isn't satisfied, to document that this case has been checked.

cjdb updated this revision to Diff 339419.Apr 21 2021, 3:53 PM

rebases so @zoecarver isn't blocked

This patch is not ready for another round of reviews

This patch is not ready for another round of reviews

FYI there is the planned changes action on Phab. That will move it out of the review queue.

cjdb updated this revision to Diff 340333.Apr 24 2021, 11:39 PM
cjdb retitled this revision from [libcxx] adds `forward_iterator` and `forward_range` to [libcxx][iterator][ranges] adds `forward_iterator` and `forward_range`.
  • moves conformance tests to appropriate files
  • adds canonical test type cxx20_forward_iterator
  • adds test for non-equality comparable iterator
cjdb updated this revision to Diff 340365.Apr 25 2021, 9:32 AM

removes old test_forward_iterator type

ldionne requested changes to this revision.Apr 28 2021, 9:37 AM

LGTM except the testing bits.

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.forward/forward_iterator.compile.pass.cpp
29

This should be removed with the new direction IIUC.

libcxx/test/support/test_iterators.h
671

I thought we had that discussion before, but why are we adding this type? Why can't we use just the existing forward_iterator? They are not materially different.

This revision now requires changes to proceed.Apr 28 2021, 9:37 AM
cjdb added a subscriber: cor3ntin.Apr 29 2021, 10:37 AM
cjdb updated this revision to Diff 341746.Apr 29 2021, 7:33 PM

rebases to activate CI

cjdb updated this revision to Diff 341958.Apr 30 2021, 10:01 AM

rebases to activate CI

zoecarver added inline comments.Apr 30 2021, 10:05 AM
libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.forward/forward_iterator.compile.pass.cpp
8

Please add two more tests:

  1. No forward_iterator_tag
  2. Not incrementable.
28

Do we still need the TODO here?

35

You don't have to take this comment, but maybe add a simple iterator that is a forward_iterator so that it's immediately obvious how the below types differ. Better yet, comment out the missing members.

45

Nit: Can you either give these all their own line or make them individual static asserts?

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.forward/subsumption.compile.pass.cpp
23

This either needs to be a libcxx only test or it needs to not use _ITER_CONCEPT.

zoecarver added inline comments.Apr 30 2021, 3:31 PM
libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.forward/forward_iterator.compile.pass.cpp
36

Also, shouldn't this type be comparable?

cjdb updated this revision to Diff 342081.Apr 30 2021, 4:39 PM
  • adds conformance tests
  • moves subsumption test from test/std to test/libcxx
cjdb updated this revision to Diff 342092.Apr 30 2021, 5:08 PM
cjdb marked 9 inline comments as done.
  • adds not_incrementable and bad_iterator_concept tests
  • removes some old code
cjdb added inline comments.Apr 30 2021, 5:18 PM
libcxx/test/support/test_iterators.h
671

Whoops, I forgot to delete this.

ldionne accepted this revision.May 3 2021, 9:28 AM

This LGTM pending CI (and the removal of XFAILs if I'm right about that).

libcxx/test/std/ranges/range.refinements/forward_range.compile.pass.cpp
13

I think we don't want those XFAILs anymore?

This revision is now accepted and ready to land.May 3 2021, 9:28 AM
cjdb updated this revision to Diff 342452.May 3 2021, 10:32 AM

removes XFAIL

zoecarver added inline comments.May 3 2021, 12:20 PM
libcxx/test/std/containers/associative/set/iterator_concept_conformance.compile.pass.cpp
17 ↗(On Diff #342452)

I think we're missing a corresponding range_conformance test for set.