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

[Tim Song via Phabricator via libcxx-commits]

tcanens 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;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101404