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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__iterator/common_iterator.h:169
+
+    if (__x_index == __y_index)
+      return true;
----------------
zoecarver wrote:
> zoecarver wrote:
> > tcanens wrote:
> > > 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.
> > > 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.
> > 
> > The difference is sentinels must always be the end of the range. I can get behind saying "this must always be true because the sentinels must always be the end of the same range which must be the same element." The part I'm having trouble with is it's OK to have two different forward iterators that point to the same range in different places. Those should return false when compared (or somehow generate an error or UB). 
> > In all cases, we need a test where we exercise that.
> 
> Tested in both assign.pass.cpp (line 35, 50, 68, 74) and eq.pass.cpp (line 78, 88, 89). 
I'd prefer to see an explicit `static_assert` here:
```
static_assert(!equality_comparable_with<_Iter, _I2>);
return true;
```
I think this assertion would sufficiently explain why we aren't checking `i1 == i2` here — it's because we physically can't. Also, the assertion serves as an important backstop against subsumption bugs. The absolute worst thing that could happen here is that the constraint on line 179 bit-rots under maintenance and we end up executing //this// code for iterators that //are// comparable.


================
Comment at: libcxx/include/__iterator/common_iterator.h:173
+    if (__x_index == 0)
+      return __unchecked_get<_Iter>(__x.__hold_) == __unchecked_get<_S2>(__y.__hold_);
+
----------------
`_VSTD::__unchecked_get<0>(__x.__hold_) == _VSTD::__unchecked_get<1>(__y.__hold_)`

I don't trust `__unchecked_get<_Iter>` to do the right thing when `_Sent` is cvref-qualified `_Iter` or vice versa (or they're in some other subtle corner-case way related to each other). Integer indices, on the other hand, are nice and solid and trustworthy. Also, cppreference documents that the code should use `get<i>` and `get<j>`, not `get<I>` and `get<S2>`; it's just barely possible that there might be a technical reason for that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103335