[libcxx-commits] [PATCH] D103335: [libcxx][ranges] Adds `common_iterator`.

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver marked 8 inline comments as done.
zoecarver added inline comments.


================
Comment at: libcxx/include/__iterator/common_iterator.h:54
+    constexpr __postfix_proxy(iter_reference_t<_Iter>&& __x)
+      : __value(__x) {}
+
----------------
tcanens wrote:
> Missing forward here.
Why not move this one too?


================
Comment at: libcxx/test/support/test_iterators.h:170
+public:
+    typedef          std::forward_iterator_tag                 iterator_category;
+    typedef typename std::iterator_traits<It>::value_type      value_type;
----------------
Quuxplusone wrote:
> zoecarver wrote:
> > ldionne wrote:
> > > Shouldn't this be `std::iterator_traits<It>::iterator_category`? Actually, we could perhaps do this instead:
> > > 
> > > ```
> > > template<class It>
> > > struct std::iterator_traits<no_default_constructor<It>> : std::iterator_traits<It> { };
> > > ```
> > > 
> > > Then, could we simply drop all the typedefs in `no_default_constructor`?
> > No, because we only provide members such that this is a forward iterator. For example, this is used mostly with pointer types. If we did what you're suggesting these would report that they were contiguous_iterators, but they are not (they're not even bidirectional). 
> Turn this inside out.
> ```
> template<class T>
> class NonDefCtablePtr {
>     T *p_;
> public:
>     using iterator_category = std::random_access_iterator_tag;
>     using value_type = T;
>     using difference_type = std::ptrdiff_t;
>     [...]
>     constexpr NonDefCtablePtr(T *p) : p_(p) {}
>     constexpr T& operator*() const { return *p_; }
>     [...]
>     friend constexpr T *base(NonDefCtablePtr p) { return p.p_; }
> };
> ```
> and then in your test cases, use
> ```
> #include "test_iterator.h"
> 
> test<bidirectional_iterator<int*>>();  // bidirectional, default-constructible
> test<bidirectional_iterator<NonDefCtablePtr<int>>>();  // bidirectional, non-default-constructible
> ```
> That is, we already have the iterator wrapper types in order to "downgrade" `int*` to forward/bidirectional/random-access level. So your `NonDefCtable` type should not "downgrade." It should just be contiguous, and then if someone wants a downgraded version, they can wrap it in one of the downgrade wrappers.
Not doing this because of Tim's comment. This is now an input iterator. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103335