Details
- Reviewers
ldionne EricWF zoecarver curdeius Mordante • Quuxplusone miscco - Group Reviewers
Restricted Project - Commits
- rGfa3e26266cd4: [libcxx][iterator][ranges] adds `forward_iterator` and `forward_range`
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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 |
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 |
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. |
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.
- moves conformance tests to appropriate files
- adds canonical test type cxx20_forward_iterator
- adds test for non-equality comparable iterator
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 ↗ | (On Diff #340365) | 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. |
libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.forward/forward_iterator.compile.pass.cpp | ||
---|---|---|
8 | Please add two more tests:
| |
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 | ||
20 | This either needs to be a libcxx only test or it needs to not use _ITER_CONCEPT. |
libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.forward/forward_iterator.compile.pass.cpp | ||
---|---|---|
36 | Also, shouldn't this type be comparable? |
libcxx/test/support/test_iterators.h | ||
---|---|---|
671 ↗ | (On Diff #340365) | Whoops, I forgot to delete this. |
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? |
libcxx/test/std/containers/associative/set/iterator_concept_conformance.compile.pass.cpp | ||
---|---|---|
17 | I think we're missing a corresponding range_conformance test for set. |
I think we're missing a corresponding range_conformance test for set.