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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__iterator/common_iterator.h:78
+  {
+    _LIBCPP_ASSERT(!__other.__hold_.valueless_by_exception(), "Preconditions violated.");
+  }
----------------
This should be in the lambda above for it to trigger before we try to access `__other.__hold_` unchecked.


================
Comment at: libcxx/include/__iterator/common_iterator.h:92
+    if (__idx == 0 && __other_idx == 0)
+      __unchecked_get<0>(__hold_) = __unchecked_get<0>(__other.__hold_);
+    else if (__idx == 1 && __other_idx == 1)
----------------
I think you want `_VSTD::__unchecked_get` here and elsewhere.


================
Comment at: libcxx/include/__iterator/common_iterator.h:97
+    // Otherwise replace with the oposite element.
+    else if (__idx == 0)
+      __hold_.template emplace<1>(__unchecked_get<1>(__other.__hold_));
----------------
tcanens wrote:
> These should look at `__other_idx` - this assignment is valid even if `*this` is valueless.
Can you add a test where you assign to a `common_iterator` with a `valueless_by_exception()` variant?


================
Comment at: libcxx/include/__iterator/common_iterator.h:107
+  {
+    _LIBCPP_ASSERT(holds_alternative<_Iter>(__hold_), "Preconditions violated.");
+    return *__unchecked_get<_Iter>(__hold_);
----------------
As discussed live: we can use a better message here.


================
Comment at: libcxx/include/__iterator/common_iterator.h:169
+
+    if (__x_index == __y_index)
+      return true;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103335