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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added subscribers: ldionne, Quuxplusone.
Quuxplusone accepted this revision as: Quuxplusone.
Quuxplusone added a comment.

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



================
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)) {}
----------------
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.


================
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>
----------------
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]).
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117950