Eliminates non_default_constructible_iterator and convertible_iterator.
Details
- Reviewers
ldionne Mordante - Group Reviewers
Restricted Project - Commits
- rGeadf7268d578: [libc++] Fix bugs in common_iterator; add test coverage.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Can you explain in the commit message why this is removed? It's not clear from the patch why it's a good idea.
Other then that it look good.
libcxx/test/std/iterators/predef.iterators/iterators.common/ctor.pass.cpp | ||
---|---|---|
0 | While we're at it, would you mind splitting the tests for these various constructors? And asking for that made me realize that we're not testing the default constructor at all (we're checking is_default_constructible but we're not actually running it). | |
1 | See, now you understand why I fixed cpp17_input_iterator to *not* be default constructible! 😄 | |
6–7 | is_convertible and is_constructible are different in that is_convertible won't work if the conversion/construction is explicit. I think this would be better: static_assert( std::is_convertible_v<DerivedIt, BaseIt>); // Derived* to Base* static_assert(!std::is_convertible_v<BaseIt, DerivedIt>); // Base* to Derived* Unless that isn't what you want (in which case I'd like to understand)? |
libcxx/test/std/iterators/predef.iterators/iterators.common/ctor.pass.cpp | ||
---|---|---|
1 | I'll admit, it was convenient. ;) | |
6–7 | I was being maybe too clever omitting "redundant" tests here: I wanted to test that DerivedIt was implicitly convertible to BaseIt, but BaseIt wasn't even explicitly convertible to DerivedIt. I could have (and now, will) test all four permutations: static_assert( std::is_convertible_v<DerivedIt, BaseIt>); // Derived* to Base* static_assert( std::is_constructible_v<BaseIt, DerivedIt>); // Derived* to Base* static_assert(!std::is_convertible_v<BaseIt, DerivedIt>); // Base* to Derived* static_assert(!std::is_constructible_v<DerivedIt, BaseIt>); // Base* to Derived* |
libcxx/test/std/iterators/predef.iterators/iterators.common/ctor.pass.cpp | ||
---|---|---|
1 |
That's correct: common_iterator<I, S> requires copyable<I> - it was the most expedient fix after relaxing the iterator requirements to allow move-only single-pass iterators. I believe it _could_ support move-only iterators by storing a variant<shared_ptr<I>, S>, but someone would need to flesh out and implement that design and write a paper. |
Well, this turned into quite the rabbit hole.
My new and improved ctor tests ended up triggering lots of missing constexpr, and then adding the constexpr (and some regression tests) found more bugs. I touched the tests that clearly needed regression tests for the bugs I was fixing. A followup PR should improve the remaining tests (and eliminate the remaining "types.h").
libcxx/docs/Status/Cxx2bIssues.csv | ||
---|---|---|
129 | Marked https://wg21.link/LWG3574 as Complete in 14.0, also. |
libcxx/test/std/iterators/predef.iterators/iterators.common/ctor_default.pass.cpp | ||
---|---|---|
1 ↗ | (On Diff #402169) | ctor.default.pass.cpp, ctor.converting.pass.cpp, etc. |
Marked https://wg21.link/LWG3574 as Complete in 14.0, also.