[libcxx-commits] [PATCH] D101242: [libcxx][nfc] prefixes test type `input_iterator` with `cpp17_`

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added inline comments.


================
Comment at: libcxx/test/support/test_iterators.h:64
 public:
     typedef          std::input_iterator_tag                   iterator_category;
     typedef typename Traits::value_type                        value_type;
----------------
Quuxplusone wrote:
> cjdb wrote:
> > Quuxplusone wrote:
> > > cjdb wrote:
> > > > Quuxplusone wrote:
> > > > > @cjdb wrote:
> > > > > > tl;dr yes. At minimum, the iterator tags are now mandatory.
> > > > > 
> > > > > The existing iterators already have tags, though. So that's not a concern for us.
> > > > > 
> > > > > I suggested to Louis in chat but will put here for the record: If the main concern of this patch is to make sure we have a test iterator which is a "non-comparable input iterator" (comparable only to its sentinel type, but not to itself), then I suggest you leave everything the way it is in main but then add the new C++20-only, sentinel-comparison-only test iterator, like this:
> > > > > ```
> > > > > #if TEST_STD_VER >= 20
> > > > > template<class It>
> > > > > class incomparable_input_iterator {
> > > > >     // This is an input iterator according to C++>=20, but not according to C++<=17.
> > > > >     // It is comparable with its sentinel type, but not with its own type.
> > > > >     [...mostly the same stuff as the regular input iterator test type...]
> > > > > public:
> > > > >     struct sentinel {
> > > > >         constexpr explicit sentinel(It it) : it_(it) {}
> > > > >     };
> > > > >     friend constexpr bool operator==(const incomparable_input_iterator& x, const sentinel& y)
> > > > >         {return x.it_ == y.it_;}
> > > > > };
> > > > > #endif
> > > > > ```
> > > > > This probably belongs right above `contiguous_iterator`, circa line 315, because we intend it to be new in C++20.
> > > > >  If the main concern of this patch is to make sure we have a test iterator which is a "non-comparable input iterator" (comparable only to its sentinel type, but not to itself)
> > > > 
> > > > This isn't the only difference between //Cpp17InputIterator// and `std::input_iterator`.
> > > > 
> > > > > I suggest you leave everything the way it is in main but then add the new C++20-only, sentinel-comparison-only test iterator
> > > > 
> > > > That leaves ambiguity as to whether or not `input_iterator` refers to a //Cpp17InputIterator// or is a strict `std::input_iterator`. It needs to be renamed to clear up that ambiguity.
> > > > This isn't the only difference
> > > 
> > > Obligatory "What are the differences you care about, then?"
> > > No two things in the world are ever exactly the same. That doesn't mean we make libc++ PRs about it. :)
> > The most salient difference is that C++20 input iterators need not be copyable. I expected that this would be fresh in your mind, since you've recently reviewed D100271, which is why I didn't mention it.
> So basically the intent of D101242-plus-D100271 is to introduce `moveonly_input_iterator` (and `moveonly_output_iterator`) while leaving everything else the same? I would approve of that direction.
> 
> But I notice that D100271 doesn't introduce a `sentinel` with which its new iterator type could be compared. It probably //should// include a sentinel, in order to make the new iterator type more useful with C++20 algorithms like `std::ranges::copy`. Still, I'm a little leery of introducing too many knobs. We probably don't want all four of a "copyable, comparable iterator" (i.e. the existing `input_iterator`/`output_iterator`) //and// a "move-only, comparable iterator" //and// a "copyable, sentineled iterator" //and// a "move-only, sentineled iterator"; but I'm still not sure what we //do// want.
> 
> (Fortunately, `std::forward_iterator` and higher all require `std::regular`, which means "copyable and comparable"; so the above combinatorial explosion applies //only// to input iterators and output iterators.)
> So basically the intent of D101242-plus-D100271 is to introduce `moveonly_input_iterator` (and `moveonly_output_iterator`) while leaving everything else the same? I would approve of that direction.

That + prefixing current `input_iterator` with `cpp17_` (or `copyable_`, which is more descriptive, but longer).

> But I notice that D100271 doesn't introduce a sentinel with which its new iterator type could be compared. It probably //should// include a sentinel, in order to make the new iterator type more useful with C++20 algorithms like `std::ranges::copy`.

The patch has no need for a sentinel, so it's not added. I think it'd be a good idea for us to think about what the new sentinel type should do, and add it either with the first algorithm or as a follow-up patch to D100271 (it certainly shouldn't be blocking).

> Still, I'm a little leery of introducing too many knobs. We probably don't want all four of a "copyable, comparable iterator" (i.e. the existing `input_iterator`/`output_iterator`) //and// a "move-only, comparable iterator" //and// a "copyable, sentineled iterator" //and// a "move-only, sentineled iterator"; but I'm still not sure what we //do// want.

Yeah, that's a bad idea. My intention is for the new, move-only input iterator is for it to be a completely minimal interface (i.e. move-only, not a self-sentinel).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101242