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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver marked 9 inline comments as done and an inline comment as not done.
zoecarver added inline comments.


================
Comment at: libcxx/include/__iterator/common_iterator.h:115
+    } else if constexpr (requires (_Iter& __i) { { *__i++ } -> __referenceable; } ||
+                         !constructible_from<iter_value_t<_Iter>, iter_reference_t<_Iter>>) {
+      return __u.__iter++;
----------------
ldionne wrote:
> http://eel.is/c++draft/iterators.common#common.iter.nav-5 says:
> 
> > Otherwise, if `requires (I& i) { { *i++ } -> can-reference; }` is `true` or `constructible_­from<iter_­value_­t<I>, iter_­reference_­t<I>> && move_­constructible<iter_­value_­t<I>>` is `false`, equivalent to: `return get<I>(v_)++;`
> 
> We're missing `&& move_constructible<...>` here?
Good catch. It looks like this was recently changed.


================
Comment at: libcxx/include/__iterator/common_iterator.h:118
+    } else {
+      __postfix_proxy __p(*this);
+      ++*this;
----------------
ldionne wrote:
> This should be `__postfix_proxy __p(**this);` (double-dereference to get the value pointed-to by the iterator).
> 
> It also means you have a hole in your coverage since your tests were passing, i.e. this branch of the `if constexpr` was never exercised. Please make sure all branches of the `if constexpr` are exercised (here and in `operator->` above too).
Yes.. you're right. I thought that `value_iterator` was testing this, but apparently not. 

I guess the type I need here is one where `i` is a `__referenceable` iterator and `++i` is a `__referenceable` iterator, but `i++` is void or something? Arg. OK.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103335