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

Thu Jul 15 09:58:25 PDT 2021

zoecarver added inline comments.

================
Comment at: libcxx/include/__iterator/common_iterator.h:35
+class common_iterator {
+  class __proxy {
+    friend common_iterator;
----------------
cjdb wrote:
> Can we define `__proxy` and `__postfix_proxy` out-of-line please?
Why?

================
Comment at: libcxx/include/__iterator/common_iterator.h:63
+public:
+  variant<_Iter, _Sent> __hold_;
+
----------------
cjdb wrote:
> This should be private. Also, `hold` is not a particularly descriptive name to me.
Because of the friend issues we discussed with transform_view on #libcxx we unfortunately can't have this be private. 

================
Comment at: libcxx/include/__iterator/common_iterator.h:122
+    requires indirectly_readable<const _I2> &&
+    (requires(const _I2& __i) { __i.operator->(); } ||
+     is_reference_v<iter_reference_t<_I2>> ||
----------------
cjdb wrote:
> Can we get this into a named object please? Perhaps `__common_iterator_has_arrow`?
@tcanens Another potential LWG issue: shouldn't this be `is_pointer_v<_Iter> || requires(const _Iter& __i) { __i.operator->(); }`? If not, do we need to have the `is_pointer` condition below? Can it ever be hit?

================
Comment at: libcxx/include/__iterator/common_iterator.h:122-124
+    (requires(const _I2& __i) { __i.operator->(); } ||
+     is_reference_v<iter_reference_t<_I2>> ||
+     constructible_from<iter_value_t<_I2>, iter_reference_t<_I2>>)
----------------
zoecarver wrote:
> cjdb wrote:
> > Can we get this into a named object please? Perhaps `__common_iterator_has_arrow`?
> @tcanens Another potential LWG issue: shouldn't this be `is_pointer_v<_Iter> || requires(const _Iter& __i) { __i.operator->(); }`? If not, do we need to have the `is_pointer` condition below? Can it ever be hit?
I don't want to add another "has arrow" concept that means something different from `__has_arrow`. Depending on my comment above, we might be able to use that here, though. 

================
Comment at: libcxx/include/__iterator/common_iterator.h:169
+
+    if (__x_index == __y_index)
+      return true;
----------------
CaseyCarter wrote:
> tcanens wrote:
> > zoecarver wrote:
> > > tcanens wrote:
> > > > cjdb wrote:
> > > > > Quuxplusone wrote:
> > > > > > 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.
> > > > > > 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?
> > > > > 
> > > > > This is the overload that's chosen when we evaluate `i == i`, so Tim's got a point. I'm also not sure what the assertion would achieve? Are you trying to prevent `i == i`?
> > > > forward iterators are required to be comparable, so it would go to the other overload.
> > > I'm talking about two completely unrelated iterators, for example, this:
> > > ```
> > >     auto iter1 = random_access_iterator<int*>(buffer + 1);
> > >     auto commonIter1 = std::common_iterator<decltype(iter1), sentinel_type<int*>>(iter1);
> > > 
> > >     auto iter2 = forward_iterator<int*>(buffer + 4);
> > >     auto commonIter2 = std::common_iterator<decltype(iter2), sentinel_type<int*>>(iter2);
> > > 
> > >     assert(commonIter1 == commonIter2);
> > > ```
> > > (An actual test case ^)
> > > 
> > > If above is UB, it's not clear to me where that is stated. Even so, if it is UB for some reason, I think it would be great to add an assertion. And if it's well defined, well that seems terrible (and my preference would be to create an LWG issue and still add an assertion or something, because that's definitely a but, but maybe others disagree). 
> > So pathological mix-up scenarios where you compare completely unrelated things? It's hard to think of a non-contrived example where that matters.
> > 
> > I don't really care about this case either way; I'm just not convinced that it's worth the trouble (coming up with something that doesn't damage valid uses can be tricky).
> > 
> > @CaseyCarter thoughts?
> It's hard to care about this. The domain of equality for forward_iterators is iterators obtained from the same range, so generic code can't perform this comparison. To do so in concrete code - when you know the result is useless - would be silly. In other words: if you think this is weird, then don't do it. 
> 
> Conversely, if you want comparison of `common_iterator<I1, S>` and `common_iterator<I2, S>` to be sensible you can always make `I1` and `I2` model `equality_comparable`.
I guess it's pathological, but it would also be pathological to even compare any iterators (read: not sentinels) that use this overload. Basically, you're saying "you should only compare iterators here that are equal." In which case, why does this overload even need to work with iterators (read: not sentinels)? 

We have two cases: 1) someone is using this correctly. They have two iterators that are equal. This returns true.

2) Someone is using this incorrectly. They have two iterators that are not equal. 

You're saying the second case isn't worth worrying about because it would be pathological. **This implies that no one should ever call this function without knowing if their iterators are equal** beforehand. This begs the question: why do they even need to call this function in the first place? If they know their iterators are equal, it's pointless to compare them. 

I think we should add a precondition or something that says i or j is 1. And I'd like to add that as an assertion here. 

================
Comment at: libcxx/include/__iterator/common_iterator.h:173
+    if (__x_index == 0)
+      return __unchecked_get<_Iter>(__x.__hold_) == __unchecked_get<_S2>(__y.__hold_);
+
----------------
Quuxplusone wrote:
> `_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.
I'm not opposed to using indexes, but a concrete example would be nice, to show that there's actually a problem. I find using types much more readable, so I'd want a solid reason not to do it. 

================
Comment at: libcxx/test/support/test_iterators.h:164
+template <class It>
+class no_default_constructor
+{
----------------
cjdb wrote:
> ldionne wrote:
> > zoecarver wrote:
> > > I think this will be a very helpful type to have now that we need to test the default constructibility of all our views/iterators.
> > I agree, but I would instead call it something like `non_default_constructible_iterator`. Or whatever you prefer, but I think it's important for the name to reflect the fact that it's an iterator.
> I think this functionality should instead be reflected in `cpp20_input_iterator` (and in a separate patch, if it breaks any code).
Common iterator requires that we're copyable, so we really need this type. But I'm fully supportive of making `cpp20_input_iterator ` non-default-constructible in a separate patch. 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103335