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

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

Quuxplusone added inline comments.


================
Comment at: libcxx/test/support/test_iterators.h:703
 
     stride_counting_iterator() = default;
 
----------------
ldionne wrote:
> 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?
We definitely still need `base(T*)`, because we want to be able to do
```
template<class It> void test() { ~~~ assert(base(it) == base(sent)); ~~~ }
int main() {
    test<std::forward_iterator<int*>>();
    test<std::random_access_iterator<int*>>();
    test<int*>();  // !!
}
```
so we need `base(it)` to be well-formed for plain pointers as well as test iterators.
(This was our only test coverage for contiguous iterators prior to C++20; but even in C++20 I think testing with non-class-type iterators i.e. pointers is still super important.)

Maybe we could get away with just a `T *base(T *p) { return p; }` instead of the full-blown `T base(T it) { return it; }` that we have today. I doubt it on principle, but certainly we can't expect `base` to do much of anything sensible if it's called on a completely arbitrary iterator (e.g. an `std::istream_iterator<int*>` or a `std::list<int>::iterator`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116613