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

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

Quuxplusone added a comment.

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



================
Comment at: libcxx/test/support/test_iterators.h:46-47
 
     TEST_CONSTEXPR It base() const {return it_;} // TODO remove me
-    friend TEST_CONSTEXPR It base(const output_iterator& i) { return i.it_; }
+    friend TEST_CONSTEXPR It base(const cpp17_output_iterator& i) { return i.it_; }
 
----------------
>> 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).


================
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>
----------------
Mordante wrote:
> 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.)
> 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//.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117950