[libcxx-commits] [PATCH] D117950: [libc++][NFC] Use cpp17_output_iterator in tests.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

In D117950#3263509 <https://reviews.llvm.org/D117950#3263509>, @Quuxplusone wrote:

> (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,()`.



================
Comment at: libcxx/test/support/test_iterators.h:36
 
-    TEST_CONSTEXPR output_iterator() : it_() {}
-    TEST_CONSTEXPR explicit output_iterator(It it) : it_(std::move(it)) {}
+    TEST_CONSTEXPR cpp17_output_iterator() : it_() {}
+    TEST_CONSTEXPR explicit cpp17_output_iterator(It it) : it_(std::move(it)) {}
----------------
Quuxplusone wrote:
> 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.
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.


================
Comment at: libcxx/test/support/test_iterators.h:55
+// formerly known as InputIterator prior to C++20. Note in C++20 these iterators are still
+// named InputIterator in several places. For example the 25.2 [algorithms.requirements].
 template <class It, class ItTraits = It>
----------------
Quuxplusone wrote:
> 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]).
> ```
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.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117950/new/

https://reviews.llvm.org/D117950