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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/test/support/test_iterators.h:701
+    using difference_type = std::iter_difference_t<It>;
+    using iterator_concept = iterator_concept_t<It>;
 
----------------
Quuxplusone wrote:
> 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.)
I will use your `conditional_t` formulation.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116613