Details
- Reviewers
ldionne EricWF zoecarver Mordante curdeius • Quuxplusone tcanens miscco - Group Reviewers
Restricted Project - Commits
- rGc05d1eed35f5: [libcxx][iterator][ranges] adds `input_iterator` and `input_range`
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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).
rebases so @zoecarver isn't blocked
This patch is not ready for another round of reviews
- replaces monolithic test with conformance tests
- adds canonical test type cxx20_input_iterator
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). |
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 | ||
321 | 😍 |
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. |
libcxx/test/support/test_iterators.h | ||
---|---|---|
638 | Actually, this is an important new test type for algorithms, iterator adaptors, and range adaptors. |
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 Since the other iterators don't change in any meaningful way, we shouldn't introduce new names for those. |
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.
Should be range const (note to reviewers: please flag these as you see them so I don't miss any).