This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Fix bugs in common_iterator; add test coverage.
ClosedPublic

Authored by Quuxplusone on Jan 15 2022, 10:31 AM.

Details

Summary

Eliminates non_default_constructible_iterator and convertible_iterator.

Diff Detail

Event Timeline

Quuxplusone requested review of this revision.Jan 15 2022, 10:31 AM
Quuxplusone created this revision.
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptJan 15 2022, 10:31 AM

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.

Quuxplusone edited the summary of this revision. (Show Details)

Also eliminate convertible_iterator since it's unused as well.

ldionne requested changes to this revision.Jan 17 2022, 11:09 AM
ldionne added inline comments.
libcxx/test/std/iterators/predef.iterators/iterators.common/ctor.pass.cpp
1

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).

34

See, now you understand why I fixed cpp17_input_iterator to *not* be default constructible! 😄

75–76

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)?

This revision now requires changes to proceed.Jan 17 2022, 11:09 AM
libcxx/test/std/iterators/predef.iterators/iterators.common/ctor.pass.cpp
34

I'll admit, it was convenient. ;)
I think what I wanted here was one default-constructible input iterator and one non-; which could have been cpp17_input_iterator and cpp20_input_iterator; except that cpp20_input_iterator is also not copyable, and I forget but I think that might disqualify it from use with common_iterator.

75–76

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*
CaseyCarter added inline comments.
libcxx/test/std/iterators/predef.iterators/iterators.common/ctor.pass.cpp
34

I think what I wanted here was one default-constructible input iterator and one non-; which could have been cpp17_input_iterator and cpp20_input_iterator; except that cpp20_input_iterator is also not copyable, and I forget but I think that might disqualify it from use with common_iterator.

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").

Quuxplusone retitled this revision from [libc++] [test] Refactor iterators.common/ctor.pass.cpp to [libc++] Fix bugs in common_iterator; add test coverage..

Fix LWG3595 while I'm at it.
Poke CI, which is already green.
Ping @ldionne!

libcxx/docs/Status/Cxx2bIssues.csv
129 ↗(On Diff #402169)

Marked https://wg21.link/LWG3574 as Complete in 14.0, also.

ldionne accepted this revision.Jan 24 2022, 10:28 AM
ldionne added inline comments.
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.

This revision is now accepted and ready to land.Jan 24 2022, 10:28 AM