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.
Details
- Reviewers
• Quuxplusone ldionne - Group Reviewers
Restricted Project - Commits
- rG5e97d37b9608: [libc++][NFC] Use cpp17_output_iterator in tests.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
(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? // 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. And of course ditto above: // This iterator meets C++20's Cpp17OutputIterator requirements, as described // in Table 90 ([output.iterators]). |
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. |
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 |
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). | |
56 |
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.) |
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. |
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.)
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.