[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/iterator_traits.h:431
 struct __has_iterator_category_convertible_to
-    : _BoolConstant<is_convertible<typename iterator_traits<_Tp>::iterator_category, _Up>::value>
 {};
----------------
Heh, thanks for fixing this :P


================
Comment at: libcxx/include/__iterator/iterator_traits.h:467
     __has_iterator_concept_convertible_to<_Tp, contiguous_iterator_tag>
 > {};
 #else
----------------
Can we make this use the concept? I guess we could make that a follow up nfc. 


================
Comment at: libcxx/include/iterator:469
 #include <__iterator/readable_traits.h>
-#include <__iterator/concepts.h>
 #include <__memory/addressof.h>
----------------
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).


================
Comment at: libcxx/include/iterator:1281
+    //
+    static_assert(__is_cpp17_random_access_iterator<_Iter>::value,
+        "__wrap_iter cannot wrap anything but a fancy pointer");
----------------
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.


================
Comment at: libcxx/include/iterator:1292
 #if _LIBCPP_STD_VER > 17
-    typedef _If<__is_cpp17_contiguous_iterator<_Iter>::value,
-                contiguous_iterator_tag, iterator_category>            iterator_concept;
+    typedef contiguous_iterator_tag                                    iterator_concept;
 #endif
----------------
If this is now unconditionally `contiguous_iterator_tag` we absolutely need to change the above assertion. 


================
Comment at: libcxx/include/iterator:1296
+            typename iterator_traits<iterator_type>::reference
+        >::type                                                        element_type;
 
----------------
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>`.


================
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 {};
 
----------------
Why is this only before C++20? Doesn't that mean we loose the optimization in C++20? 


================
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());
----------------
Do we still need this? I think not. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101404