This is an archive of the discontinued LLVM Phabricator instance.

[libc++][NFC] Use cpp17_output_iterator in tests.
ClosedPublic

Authored by Mordante on Jan 22 2022, 7:41 AM.

Details

Summary

The renames the output_iterator to cpp17_output_iterator. These
iterators are still used in C++20 so it's not possible to change the
current type to the new C++20 requirements. This is done in a similar
fashion as the cpp17_input_iterator.

Diff Detail

Event Timeline

Mordante created this revision.Jan 22 2022, 7:41 AM
Mordante requested review of this revision.Jan 22 2022, 7:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 22 2022, 7:41 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

(I assume every diff outside of test_iterators.h is just s/output_iterator/cpp17_output_iterator/, right?)

I think this is fine. I'd kinda like to see the roadmap, though: is your next step to introduce a cpp20_output_iterator? What will its properties be? (I'd guess "move-only, non-default-constructible, non-equality-comparable, value_type=void, difference_type=ptrdiff_t" ...what other knobs am I forgetting?) I think the best way to communicate that would be to just go ahead and make that PR, as a child of this one.

libcxx/test/support/test_iterators.h
35–36

FWIW, I suspect that @ldionne would like to see this default constructor removed, since it does not appear in the Cpp17OutputIterator requirements. But that would stop this PR from being "NFC", so, no action required for now.

56

You're talking about http://eel.is/c++draft/algorithms.requirements#4.1.sentence-1 , right?
The paper standard basically makes teletype InputIterator a synonym for italicized Cpp17InputIterator; but I don't think there's any formal inconsistency there (I mean it's not just a typo or oversight, as this comment seems to imply). Actually, I would trim the pre-existing comment here to just

// This iterator meets C++20's Cpp17InputIterator requirements, as described
// in Table 89 ([input.iterators]).

If someone wants to know the history of the name, they can go look.
I changed "the" to "C++20's" to avoid bit-rot. I observe that the table number has already bit-rotted (Table 87 is now Table 89).

And of course ditto above:

// This iterator meets C++20's Cpp17OutputIterator requirements, as described
// in Table 90 ([output.iterators]).

(I assume every diff outside of test_iterators.h is just s/output_iterator/cpp17_output_iterator/, right?)

yes.

I think this is fine. I'd kinda like to see the roadmap, though: is your next step to introduce a cpp20_output_iterator? What will its properties be? (I'd guess "move-only, non-default-constructible, non-equality-comparable, value_type=void, difference_type=ptrdiff_t" ...what other knobs am I forgetting?) I think the best way to communicate that would be to just go ahead and make that PR, as a child of this one.

I probably make that diff after this one has landed. I basically want to make an iterator matching the output_iterator concept not much more. Obviously it will get base() member function and a deleted operator,().

libcxx/test/support/test_iterators.h
35–36

I'm indeed opposing that change in this commit, since it's huge. I don't mind to make a separate review to make that change, then it's visible.

56

The current comment implies InputIterator in C++20 has a different meaning, which it does not.
(indeed numbes bitrot fast, but the table name is there too, making it still easy to find.)

To be clear, I'm pretty sure you should wait for @ldionne's green light before landing this, just in case he has opinions. :)

libcxx/test/support/test_iterators.h
45–46

I think this is fine. I'd kinda like to see the roadmap, though: is your next step to introduce a cpp20_output_iterator? What will its properties be? (I'd guess "move-only, non-default-constructible, non-equality-comparable, value_type=void, difference_type=ptrdiff_t" ...what other knobs am I forgetting?) I think the best way to communicate that would be to just go ahead and make that PR, as a child of this one.

I probably make that diff after this one has landed. I basically want to make an iterator matching the output_iterator concept not much more. Obviously it will get base() member function and a deleted operator,().

Nit: Please give it an ADL base function, not a member function. As you see on line 46 above, we're trying to get rid of these .base() members because some Ranges iterators have actual .base() members and the overloading of the name is confusing (for humans, that is, not for the computer).
One of my goals for today is to finally write that "There's no such thing as an archetype" blog post. I think you'll find it's literally impossible to make an iterator that "matches the output_iterator concept not much more," because the output_iterator concept has so many knobs (some of which I mentioned in my comment).

56

The current comment implies InputIterator in C++20 has a different meaning, which it does not.

Agreed (I think); that's why I recommended trimming the current comment to just

// This iterator meets C++20's Cpp17InputIterator requirements, as described
// in Table 89 ([input.iterators]).

We don't need to mention anything about InputIterator (or InputIterator), because those aren't a thing; anyone grepping for InputIterator in the standard will be led to the Cpp17InputIterator requirements, and that's fine. (They won't be grepping as a result of anything we say here, because we're not going to mention InputIterator here. Just Cpp17InputIterator.)

ldionne accepted this revision.Feb 2 2022, 12:51 PM

I probably make that diff after this one has landed. I basically want to make an iterator matching the output_iterator concept not much more. Obviously it will get base() member function and a deleted operator,().

Good -- and then I assume we'll want to revisit all the places where we have tests running on cpp17_output_iterator and also run them on the new output_iterator (the C++20 one you'll have just introduced)? Furthermore, I assume we have the same problem for all other iterator "archetypes". We could in theory have the Cpp17FooIterator version and the C++20 version for each of those. I'm not asking that you sign up to do those, though, I understand you're probably interested in output_iterator because of std::format.

This LGTM. I'd be in favour of using "C++20's Cpp17InputIterator" in the wording of the comments, but this is non-blocking, just a preference that increases clarity IMO. Thanks for noticing this and making the change.

libcxx/test/support/test_iterators.h
35–36

Yes, indeed, I would like to see it removed so the "archetype" is more minimal. (Indeed, there's no such thing as an archetype, but we can still strive for minimalism).

I also agree that this change has nothing to do in this commit. @Mordante you can look into it in a later patch if you have an appetite for it.

50–53

Would it make sense to static_assert(std::output_iterator<cpp17_output_iterator<int*>>); here, as a statement that a Cpp17OutputIterator also satisfies the requirements of a C++20 output_iterator?

56

FWIW, I too like using "C++20's Cpp17InputIterator requirements [...]" because it adds precision IMO.

This revision is now accepted and ready to land.Feb 2 2022, 12:51 PM
Mordante updated this revision to Diff 405528.Feb 2 2022, 11:00 PM

Rebased to see whether CI still passes.

Mordante marked 7 inline comments as done.Feb 2 2022, 11:01 PM

Good -- and then I assume we'll want to revisit all the places where we have tests running on cpp17_output_iterator and also run them on the new output_iterator (the C++20 one you'll have just introduced)? Furthermore, I assume we have the same problem for all other iterator "archetypes". We could in theory have the Cpp17FooIterator version and the C++20 version for each of those. I'm not asking that you sign up to do those, though, I understand you're probably interested in output_iterator because of std::format.

It's indeed motivated by std::format. I'm using some algorithms, for example std::fill_n, which don't work with move-only iterators. So I either need to implement a move-only version myself or improve the existing algorithms. I think the improvement can be useful for users of the algorithms. (I'm actually surprised the algorithm requirements haven't changed in C++20.)

Mordante updated this revision to Diff 405569.Feb 3 2022, 3:39 AM

Fix the CI.

Mordante updated this revision to Diff 405653.Feb 3 2022, 8:19 AM

Update recently added usage of output_iterator.

This revision was automatically updated to reflect the committed changes.
libcxx/test/std/localization/locale.categories/category.numeric/locale.nm.put/facet.num.put.members/put_long.pass.cpp