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

Wed Feb 2 12:51:10 PST 2022

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

In D117950#3264341 <https://reviews.llvm.org/D117950#3264341>, @Mordante wrote:

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

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

================
Comment at: libcxx/test/support/test_iterators.h:51
     void operator,(T const &) = delete;
 };

----------------
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?

================
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:
> 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//.)
FWIW, I too like using "C++20's `Cpp17InputIterator` requirements [...]" because it adds precision IMO.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117950