[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:169
+
+    if (__x_index == __y_index)
+      return true;
----------------
zoecarver wrote:
> tcanens wrote:
> > zoecarver wrote:
> > > 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.
> > This is meant for C++20 input iterators that aren't comparable with each other. Because incrementing an input iterator invalidates all copies, you can't have two valid iterators pointing to different elements in the same range. 
> > 
> > > We should assert these aren't both zero, even though it's non-conforming, people will thank us.
> > 
> > Please don't. `i == i` had better work.
> > This is meant for C++20 input iterators that aren't comparable with each other. Because incrementing an input iterator invalidates all copies, you can't have two valid iterators pointing to different elements in the same range.
> 
> Shouldn't this assert (or require) that these are input iterators then?
> 
> > Please don't. i == i had better work.
> 
> But this is not i == i, it's x == y. What you're saying makes sense for input iterators, but what about forward iterators, or even contiguous iterators? 
> 
> (Thanks for all the help with interpreting the standard's wording here, by the way.)
`common_iterator` is a C++17 compatibility shim. I don't think this is worse than returning `true` when both are sentinels - two sentinels can mean completely different things, even if they have the same type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103335