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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added inline comments.


================
Comment at: libcxx/include/__iterator/common_iterator.h:40
+    constexpr __proxy(iter_reference_t<_Iter>&& __x)
+      : __value(_VSTD::move(__x)) {}
+
----------------
Add a comment: "iter_reference_t" is never a reference here because if it is we catch it in the if constexpr branch above where this one is called.




================
Comment at: libcxx/include/__iterator/common_iterator.h:107
+  {
+    _LIBCPP_ASSERT(holds_alternative<_Iter>(__hold_), "Preconditions violated.");
+    return *__unchecked_get<_Iter>(__hold_);
----------------
This should say something like "common_iterator not holding an iterator (holding a sentinel) must hold iterator."




================
Comment at: libcxx/include/__iterator/common_iterator.h:169
+
+    if (__x_index == __y_index)
+      return true;
----------------
ldionne wrote:
> How is this behavior really what we want? IIUC, that means that if you compare two `common_iterators` that both contain different iterators (say `It1` and `It2`) that are not comparable to each other, the `common_iterator`s will compare equal to each other even if `It1` and `It2` are "pointing" to entirely different elements. Am I misunderstanding something here, or that's an incredibly subtle (and broken) behavior to have at runtime? @tcanens Can you shed some light on this?
> 
> In all cases, we need a test where we exercise that.
We should assert these aren't both zero, even though it's non-conforming, people will thank us.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103335