[libcxx-commits] [PATCH] D116613: [libc++] Refactor stride_counting_iterator

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision as: libc++.
ldionne added a comment.
This revision is now accepted and ready to land.

Will ship once CI is green, thanks for the discussion @Quuxplusone .



================
Comment at: libcxx/test/support/test_iterators.h:703
 
     stride_counting_iterator() = default;
 
----------------
Quuxplusone wrote:
> ldionne wrote:
> > Quuxplusone wrote:
> > > I believe your intent here was (or should have been ;))
> > > ```
> > >   explicit stride_counting_iterator() requires std::default_initializable<It> = default;
> > > ```
> > > because after all your efforts to make `cpp17_input_iterator<int*>` non-default-constructible, it wouldn't make sense if `stride_counting_iterator<cpp17_input_iterator<int*>>` were default-constructible again.
> > > 
> > > However, I think this also highlights an obstacle with this entire design (both old and new). You want `stride_counting_iterator<It>` to expose all the same API surface as `It`, but wrapped, and that's just physically very hard to achieve — requires all this metaprogramming. I can't help wondering if there's a simpler approach.
> > > Have you tried simply writing a `stride_counter<T*>` that wraps a pointer and is always contiguous, and then using types like `forward_iterator<stride_counter<T*>>` in the tests (instead of `stride_counter<forward_iterator<int*>>`)?  That violates our previously unwritten rule that `forward_iterator<It>` is only ever instantiated with `T*`; but it doesn't violate it //too// much, does it? Or do we end up with tons of cryptic errors?
> > I'm going to take the `requires default_initializable` change. I think trying to commute the stride counting and the forward-iteratorness (or other archetype) is a great idea. I've vaguely considered it in the past too, but it seemed like a lot of work upfront and I could hardly justify it to myself.
> > 
> > I still think this patch is an improvement as-is, so I'd like to land it without expanding the scope so much. I think we can get to where we both want incrementally. First, we can make `base()` call itself recursively so that it unwraps all levels of iterator-ness. Then, we can try to commute `stride_counting_iterator`.
> > I still think this patch is an improvement as-is, so I'd like to land it without expanding the scope so much. I think we can get to where we both want incrementally.
> 
> Ack, and go ahead.
> 
> What follows is just preemptive nitpicking/punditry on the rest of your plan:
> 
> > First, we can make base() call itself recursively so that it unwraps all levels of iterator-ness.
> 
> I don't think we want base() to call itself recursively. I've discussed an exactly isomorphic problem with the author of https://oleksandrkvl.github.io/2021/10/11/projected-ranges.html — he calls the single step `.base()` and the recursive version `.root()`. I maintain that `.root()` is simply useless in generic code: You get a gift. You put it inside a box, maybe several nested boxes. Then you call `.root()` to unwrap all the boxes — but you won't get back your original gift //in the case that your original gift was itself wrapped in a box.//
> Instead, I think we want each test iterator to have a clear idea of how it's supposed to be used: e.g. what is its type parameter supposed to be (only `T*`? any test iterator from the prior level? any test iterator at all? any //iterator// at all?). Then, its ADL `base(x)` won't need to be recursive; it should have a clear idea of exactly how many layers (if any) need to be unwrapped and/or re-applied inside `base`.
> 
> I still think it makes sense to have `base(sentinel_wrapper<forward_iterator<T*>>)` return `T*` instead of `forward_iterator<T*>`. But it's not going to do that because "recursion"; it's going to do that because we want `base(it) == base(sent)` to be well-formed.
> 
> > Then, we can try to commute stride_counting_iterator.
> 
> +1.
> I still think it makes sense to have `base(sentinel_wrapper<forward_iterator<T*>>)` return `T*` instead of `forward_iterator<T*>`. But it's not going to do that because "recursion"; it's going to do that because we want `base(it) == base(sent)` to be well-formed.

Interesting, but in that case I would argue that defining `base(T*)` as being the identity is breaking this philosophy. `base(T*)` is acting exactly as the "end of the recursion" in our unwrapping right now, and it's really convenient. Maybe defining `base(T*)` won't be necessary at all if we use your approach?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116613