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

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__iterator/iterator_traits.h:467
     __has_iterator_concept_convertible_to<_Tp, contiguous_iterator_tag>
 > {};
 #else
----------------
zoecarver wrote:
> Can we make this use the concept? I guess we could make that a follow up nfc. 
Absolutely not, because that would break all C++17 fancy pointers when compiled in C++20 mode. We can't assume that everyone running the compiler in C++20 mode is actually able to use `contiguous_iterator_tag`.


================
Comment at: libcxx/include/iterator:469
 #include <__iterator/readable_traits.h>
-#include <__iterator/concepts.h>
 #include <__memory/addressof.h>
----------------
zoecarver wrote:
> Please keep this as per D101098:
> > All sub-headers should be included by their parent header. To use the example above, ``<ranges>`` must include the sub-header ``__ranges/size.h``, even if it  doesn't use ``ranges::size`` itself. This inclusion is mandatory because the standard requires the top level header to provide these declarations. This rule falls out of the more general "Include What You Use." You should never rely on some other header to transitively include something that you need. If your header needs a definition of ``iterator_traits``, then you should either ``#include <iterator>`` (exactly as user code does), or, to improve compile time, ``#include <__iterator/iterator_traits.h>``. You should never expect ``iterator_traits`` to be "pulled in" by any other header.
> 
> (Excuse the formatting).
Yes, I know. Check alphabetization and duplication. (This trivial diff will disappear soon because I'll have landed it.)


================
Comment at: libcxx/include/iterator:1281
+    //
+    static_assert(__is_cpp17_random_access_iterator<_Iter>::value,
+        "__wrap_iter cannot wrap anything but a fancy pointer");
----------------
zoecarver wrote:
> Who gets broken by making this `__is_cpp17_contiguous_iterator`? This is only used to wrap pointers and internal iterators. We should just make it work.
All C++17 fancy pointers get broken by that. C++17 fancy pointers do not advertise themselves as contiguous iterators, because they cannot; there is no way to advertise yourself as a contiguous iterator in C++17-or-earlier.


================
Comment at: libcxx/include/iterator:1296
+            typename iterator_traits<iterator_type>::reference
+        >::type                                                        element_type;
 
----------------
zoecarver wrote:
> Why can't we use `pointer_traits` with this? That would seem more correct to me. Especially if we're advertising this as a "known trivial wrapper around a pointer type." 
> 
> The whole reason we want to have an element type is so that `pointer_traits` is correct for `__warp_iter`. If our wrapped type doesn't support `pointer_traits` then we really shouldn't either, because we'll be falsely "supporting" something that we don't actually "support." However, as I have said elsewhere, I don't think we actually wrap any types that don't support `pointer_traits`, so I think we are safe to use that (maybe with a static assert). 
> 
> If nothing else, let's make this `remove_pointer<iterator_traits::pointer>`.
As the comment says, "Prior to C++20, there was no "standard" way to mark a type as a fancy pointer; after C++20, we would prefer all fancy pointers to have contiguous_iterator_tag, but we can't assume that they will, because of legacy code."
In particular, it is not safe to use `pointer_traits<FancyPointer>` in C++20 mode, because not all C++17 FancyPointers are safe to use with `pointer_traits` in C++20 mode. This is tested (sorta) via the test for `__wrap_iter<my_random_access_iterator>` in libcxx/test/libcxx/iterators/contiguous_iterators.pass.cpp.


================
Comment at: libcxx/include/iterator:1478
 template <class _It>
-struct __is_cpp17_contiguous_iterator<__wrap_iter<_It> > : __is_cpp17_contiguous_iterator<_It> {};
-#endif
+struct __is_cpp17_contiguous_iterator<__wrap_iter<_It> > : true_type {};
 
----------------
zoecarver wrote:
> Why is this only before C++20? Doesn't that mean we loose the optimization in C++20? 
In C++20 and later, `__is_cpp17_contiguous_iterator<__wrap_iter<_It>>` is already true, because `__wrap_iter<_It>` advertises itself as a contiguous iterator in C++20. The specialization is needed only in C++17-and-earlier, because in those modes there is no established way for `__wrap_iter` to advertise itself as a contiguous iterator.


================
Comment at: libcxx/include/iterator:1483
+decltype(_VSTD::__to_address(declval<_Iter>()))
 __to_address(__wrap_iter<_Iter> __w) _NOEXCEPT {
     return _VSTD::__to_address(__w.base());
----------------
zoecarver wrote:
> Do we still need this? I think not. 
We do.


================
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;
----------------
zoecarver wrote:
> 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?
@CaseyCarter: I was looking at https://godbolt.org/z/f4zbjMMoz and seeing that `pointer_traits<std::vector<int>::iterator>::element_type` doesn't work at all.
Similarly with the static_assert you posted, https://godbolt.org/z/jGGEEaE5a , if compiled in any mode other than C++20 mode.
Ditto for GCC's libstdc++: https://godbolt.org/z/75sqxnKKa
However, I'm still inclining toward making everything Just Work for libc++. Otherwise, it's too confusing for  my brain to keep track of what "works," what "doesn't work on purpose," what "doesn't work not on purpose," etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101404