[libcxx-commits] [PATCH] D101404: [libc++] Comment preconditions for __wrap_iter; make it work with C++20 to_address

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added inline comments.


================
Comment at: libcxx/include/iterator:1280
     typedef typename iterator_traits<iterator_type>::pointer           pointer;
+    typedef typename pointer_traits<pointer>::element_type             element_type;
     typedef typename iterator_traits<iterator_type>::reference         reference;
----------------
tcanens wrote:
> CaseyCarter wrote:
> > Quuxplusone wrote:
> > > tcanens wrote:
> > > > zoecarver wrote:
> > > > > Quuxplusone wrote:
> > > > > > If my comments are correct, then this could+should use `pointer_traits<_Iter>` instead of `pointer_traits<pointer>`...
> > > > > > ...and the `_If` on line 1284 is unconditionally true, and can be simplified.
> > > > > > Please try that simplification (at least locally) and see if it passes the test suite.
> > > > > Alternatively we could add `element_type` with an `_If` (or maybe inheritance?) to make this work for non-pointer-like iterators. 
> > > > It's surprising to me that this is even needed. What breaks without this change?
> > > In the old code, `pointer_traits<std::vector<int>::iterator>::element_type` would be `int*`, not `int`. https://godbolt.org/z/PTPvn11bf
> > > Providing a user-provided `element_type` fixes that (in both C++17 and C++20).
> > > 
> > > Interestingly, libstdc++ and MSVC STL //also// report `pointer_traits<std::vector<int>::iterator>::element_type` as something other than `int`. I don't know if this is a conscious decision ("contiguous iterators needn't support pointer_traits") or if they just haven't gotten around to that part of C++20 yet. @CaseyCarter ?
> > https://godbolt.org/z/ba6z9cEbe demonstrates fairly clearly that `pointer_traits<vector<T>::iterator>::element_type` is `T` for MSVC STL. It looks like GCC didn't need to specialize `pointer_traits` for the `to_address` requirement: presumably `operator->` always works with their pointer-wrapper type. This is not the case for our `_Vector_iterator` which asserts in debug mode if you call `operator->` on a non-dereferenceable iterator.
> Except that nothing in the standard requires `pointer_traits<std::vector<int>::iterator>::element_type` to be meaningful, only that it doesn't explode. So what was broken?
> 
> If you want to claim that it is better QOI, that's fine (but since lots of - even most of - contiguous iterators are not even //Cpp17NullablePointer//s I find using `pointer_traits` with them rather debatable), but that has nothing to do with `to_address` really.
That's a good point. Our implementation of `to_address` shouldn't care what `element_type` is so long as it exists. For `__wrap_iter` it should be using `operator->` because `pointer_traits<__wrap_iter>::to_address` doesn't exist either way. Maybe this patch isn't the right fix (though I think the cleanups should be applied regardless).

We should investigate why we can't use `to_address` with the following type:
```
struct T {
  using element_type = int*;
  int *operator->();
};
```

https://godbolt.org/z/ed7Pqzd8n

@quuxplusone thoughts?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101404