[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:707
 
-    constexpr const I& base() const& { return base_; }
-
-    constexpr I base() && { return std::move(base_); }
+    friend constexpr It base(stride_counting_iterator const& it) { return It(it.base_); }
 
----------------
Quuxplusone wrote:
> If you don't take my advice about flipping to `forward_iterator<stride_counter<T*>>`, so you keep the tests using `stride_counting_iterator<forward_iterator<int*>>`, then I suggest investigating whether `base(sci<fi<int*>>)` should simply return the `int*` itself instead of a `fi<int*>`. That would eliminate a lot of `base(base(` cruft in the tests.
> I wonder if `sentinel_wrapper` and `sized_sentinel` should also be done that way. I must have considered it circa D115766 and decided not to, but I don't remember why; it might have just been to avoid churn in the places that used the old code.
> I wonder if sentinel_wrapper and sized_sentinel should also be done that way.

This morning I found some concrete evidence that they should be done that way. With the current system, we have
```
struct Common {
    forward_iterator<int*> begin() const;
    forward_iterator<int*> end() const;
};
struct Uncommon {
    forward_iterator<int*> begin() const;
    sentinel_wrapper<forward_iterator<int*>> end() const;
};
Common c;
Uncommon uc;
assert(base(c.begin()) == base(c.end()));  // Common: Nice!
assert(base(uc.begin()) == base(base(uc.end())));  // Uncommon: Ugly!
```
If ADL `base(sentinel)` returned the iterator's base type, then we'd be able to use the "Nice!" code for both `Common` and `Uncommon`. Which would be nice, especially in (hypothetically) templated code and (pragmatically) copy-pasted code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116613