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

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

Quuxplusone requested changes to this revision.
Quuxplusone added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/test/support/test_iterators.h:701
+    using difference_type = std::iter_difference_t<It>;
+    using iterator_concept = iterator_concept_t<It>;
 
----------------
This is the only place `iterator_concept_t` is used. Could we change this to something like
```
    using iterator_concept = std::conditional_t<
        std::contiguous_iterator<It>, std::contiguous_iterator_tag, std::conditional_t<
        std::random_access_iterator<It>, std::random_access_iterator_tag, std::conditional_t<
        std::bidirectional_iterator<It>, std::bidirectional_iterator_tag, std::conditional_t<
        std::forward_iterator<It>, std::forward_iterator_tag, std::conditional_t<
        std::input_iterator<It>, std::input_iterator_tag, std::output_iterator_tag
    >>>>>;
```
and then eliminate lines 649–689?
Alternatively, is it possible to replace the whole thing with just
```
    using iterator_category = std::iterator_traits<It>::iterator_category;
```
or does that fail tests such as
```
static_assert(std::contiguous_iterator<stride_counting_iterator<int*>>);
```
? (This would all be //much// easier if //`ITER-CONCEPT`// were a real thing instead of being exposition-only.)


================
Comment at: libcxx/test/support/test_iterators.h:703
 
     stride_counting_iterator() = default;
 
----------------
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?


================
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_); }
 
----------------
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.


================
Comment at: libcxx/test/support/test_iterators.h:715
 
-    constexpr decltype(auto) operator[](difference_type const n) const { return base_[n]; }
+    constexpr decltype(auto) operator[](difference_type const n) const { return It(base_)[n]; }
 
----------------



================
Comment at: libcxx/test/support/test_iterators.h:799-803
+    template <std::sentinel_for<It> Sent>
+    constexpr bool operator==(Sent last) const
     {
-        return base_ == last;
+        return It(base_) == last;
     }
----------------
This overload should go away. Well-formedness of `It == Sent` doesn't imply `stride_counter<It> == Sent`; those types are unrelated. In tests, we should only ever use `stride_counter<It> == sentinel_wrapper<stride_counter<It>>`.

(Notice that the old code failed to provide an overload for `stride_counter<It> - Sent`; that's good IMO, because if a test wants a sized sentinel they should be using `stride_counter<It> - sized_sentinel<stride_counter<It>>`.)


================
Comment at: libcxx/test/support/test_iterators.h:811-815
     friend constexpr bool operator>(stride_counting_iterator const& x, stride_counting_iterator const& y)
-        requires std::random_access_iterator<I>
+        requires std::random_access_iterator<It>
     {
         return y < x;
     }
----------------
Scope creep, feel free to disregard or defer; but for the record, I'd love to see these relops rewritten to use a more copy-pastey style:
```
    friend constexpr bool operator<(stride_counting_iterator const& x, stride_counting_iterator const& y)
        requires std::random_access_iterator<It> {
        return It(x.base_) < It(y.base_);
    }

    friend constexpr bool operator<=(stride_counting_iterator const& x, stride_counting_iterator const& y)
        requires std::random_access_iterator<It> {
        return It(x.base_) <= It(y.base_);
    }

    friend constexpr bool operator>(stride_counting_iterator const& x, stride_counting_iterator const& y)
        requires std::random_access_iterator<It> {
        return It(x.base_) > It(y.base_);
    }
```
and so on, where the only token that changes from function to function is the spelling of the operator itself.
I now observe that I never pulled out and submitted a PR to change all the existing test iterators' relops to hidden friends. (`contiguous_iterator` uses this style already, but I mean to make the others do so also. I'll go submit that today.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116613