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

[Tim Song via Phabricator via libcxx-commits]

tcanens added inline comments.


================
Comment at: libcxx/include/__iterator/common_iterator.h:41
+    constexpr __proxy(iter_reference_t<_Iter>&& __x)
+      : __value(__x) {}
+
----------------
Missing move here.


================
Comment at: libcxx/include/__iterator/common_iterator.h:54
+    constexpr __postfix_proxy(iter_reference_t<_Iter>&& __x)
+      : __value(__x) {}
+
----------------
Missing forward here.


================
Comment at: libcxx/include/__iterator/common_iterator.h:71
+  constexpr common_iterator(const common_iterator<_I2, _S2>& __other)
+    : __hold(__other.__hold) {}
+
----------------
 `variant` does not have heterogeneous conversions of this sort.


================
Comment at: libcxx/include/__iterator/common_iterator.h:77
+  common_iterator& operator=(const common_iterator<_I2, _S2>& __other) {
+    __hold = __other.__hold;
+  }
----------------
Likewise.


================
Comment at: libcxx/include/__iterator/common_iterator.h:80-83
+  decltype(auto) operator*() { return *_VSTD::get<_Iter>(__hold); }
+  decltype(auto) operator*() const
+    requires __dereferenceable<const _Iter>
+  { return *_VSTD::get<_Iter>(__hold); }
----------------
It's undefined if it is holding the wrong type, so the check-and-throw `get` performs is unnecessary (and hurts performance). There should be a way to avoid such checks.


================
Comment at: libcxx/include/__iterator/common_iterator.h:124
+    if (__x.__hold.index() == 0)
+      return _VSTD::get<_Iter>(__x.__hold) == _VSTD::get<_Sent>(__y.__hold);
+
----------------
`__y`'s sentinel type is `_S2`, not `_Sent`. Ditto below.

There seems to be a lack of test coverage here.


================
Comment at: libcxx/test/std/iterators/predef.iterators/iterators.common/plus_plus.pass.cpp:87
+    // Note: operator++ returns void.
+    // assert(*(commonIter1++) == 1);
+    assert(*commonIter1 == 2);
----------------
zoecarver wrote:
> @tcanens and others, is this correct? It seems like this goes against the other logic here (isn't the whole point to make sure that operator++ always returns a referencable type)?
Yes, `common_iterator` does its best to return something from postfix `++` that meets the C++17 input iterator requirements. 

But hitting this case means that it doesn't know a way to produce anything meaningful, so it just gives up.


================
Comment at: libcxx/test/support/test_iterators.h:171
+public:
+    typedef          std::forward_iterator_tag                 iterator_category;
+    typedef typename std::iterator_traits<It>::value_type      value_type;
----------------
Something that's not default constructible can't be anything more than input.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103335