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

[Zoe Carver via Phabricator via libcxx-commits]

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


================
Comment at: libcxx/include/__iterator/common_iterator.h:156
+    requires sentinel_for<_Sent, _I2> && equality_comparable_with<_Iter, _I2>
+  friend bool operator==(const common_iterator& __x, const common_iterator<_I2, _S2>& __y) {
+    if (__x.__hold_.index() == 1 && __y.__hold_.index() == 1)
----------------
ldionne wrote:
> Please make sure you test all the branches below (here and in the other functions below too).
Added a few tests. Have verified all paths are taken. 


================
Comment at: libcxx/test/std/iterators/predef.iterators/iterators.common/iterator_traits.compile.pass.cpp:56
+    static_assert(std::same_as<IterTraits::difference_type, std::ptrdiff_t>);
+    static_assert(std::same_as<IterTraits::pointer, const Iter&>); // TODO: is this right?
+    static_assert(std::same_as<IterTraits::reference, int&>);
----------------
ldionne wrote:
> This doesn't look right. According to the spec (http://eel.is/c++draft/iterators.common#common.iter.types-1.3), this should either be the type of `operator->()` or `void`, but never `Iter const&`. This means you most likely have a bug in the implementation.
> 
> Also, please add a test where `pointer` ends up being `void`.
> This doesn't look right. According to the spec (http://eel.is/c++draft/iterators.common#common.iter.types-1.3), this should either be the type of operator->() or void, but never Iter const&. This means you most likely have a bug in the implementation.

Turns out this is actually correct. This is supposed to be the return type of **common_iterator**'s `operator->()`. And if `_Iter` has an `operator->()` then it's supposed to simply return `get<Iter>(v)`. (However, I'd argue that should maybe return `get<Iter>(v).operator->()`, if you agree, maybe I'll file an LWG issue.)

Sound good now? This threw me off too :P


================
Comment at: libcxx/test/std/iterators/predef.iterators/iterators.common/iterator_traits.compile.pass.cpp:56
+    static_assert(std::same_as<IterTraits::difference_type, std::ptrdiff_t>);
+    static_assert(std::same_as<IterTraits::pointer, const Iter&>); // TODO: is this right?
+    static_assert(std::same_as<IterTraits::reference, int&>);
----------------
zoecarver wrote:
> ldionne wrote:
> > This doesn't look right. According to the spec (http://eel.is/c++draft/iterators.common#common.iter.types-1.3), this should either be the type of `operator->()` or `void`, but never `Iter const&`. This means you most likely have a bug in the implementation.
> > 
> > Also, please add a test where `pointer` ends up being `void`.
> > This doesn't look right. According to the spec (http://eel.is/c++draft/iterators.common#common.iter.types-1.3), this should either be the type of operator->() or void, but never Iter const&. This means you most likely have a bug in the implementation.
> 
> Turns out this is actually correct. This is supposed to be the return type of **common_iterator**'s `operator->()`. And if `_Iter` has an `operator->()` then it's supposed to simply return `get<Iter>(v)`. (However, I'd argue that should maybe return `get<Iter>(v).operator->()`, if you agree, maybe I'll file an LWG issue.)
> 
> Sound good now? This threw me off too :P
> Also, please add a test where pointer ends up being void.

Done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103335