[libcxx-commits] [PATCH] D130835: [libc++] Fix a hard error in `contiguous_iterator<NoOperatorArrowIter>`.

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
Comment at: libcxx/include/__memory/pointer_traits.h:197
 template <class _Pointer, class = __enable_if_t<
-    !is_pointer<_Pointer>::value && !is_array<_Pointer>::value && !is_function<_Pointer>::value
+    _And<is_class<_Pointer>, _IsFancyPointer<_Pointer> >::value
 > >
----------------
ldionne wrote:
> var-const wrote:
> > var-const wrote:
> > > This is more to start a discussion. Would it be easier to list what the `_Pointer` could be rather than what it should *not* be? Are there any other alternatives rather than a class that we would accept here?
> > `_And` is necessary because `enable_if` doesn't short-circuit, so it would still try to evaluate `_IsFancyPointer` on e.g. an array, leading to a hard error when trying to instantiate `pointer_traits<array-type>`.
> Here's a refactoring suggestion:
> 
> ```
> template <class _Pointer, class = __enable_if_t<
>     _And<is_class<_Pointer>, _HasToAddress<_Pointer> >::value
> > >
> _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
> decay_t<decltype(pointer_traits<_Pointer>::to_address(declval<const _Pointer&>()))>
> __to_address(const _Pointer& __p) _NOEXCEPT {
>     return pointer_traits<_Pointer>::to_address(__p);
> }
> 
> template <class _Pointer, class = __enable_if_t<
>     _And<is_class<_Pointer>, _HasArrow<_Pointer>, _Not<_HasToAddress<_Pointer> > >::value
> > >
> _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
> decltype(std::__to_address(declval<const _Pointer&>().operator->()))
> __to_address(const _Pointer& __p) _NOEXCEPT {
>     return std::__to_address(__p.operator->());
> }
> ```
> 
> Something like that. And then we could remove `__to_address_helper` altogether -- it feels like it's kind of unnecessary.
Hmm, it looks like the compiler is having trouble with
```
decltype(std::__to_address(declval<const _Pointer&>().operator->()))
```
when the type returned by `_Pointer::operator->` is another fancy pointer with `operator->` defined -- looks like the recursion is causing the issue. `decltype(auto)` works but we can't use it because of the C++03 mode. I wonder if that's the reason the `helper` struct was added in the first place?


================
Comment at: libcxx/include/__memory/pointer_traits.h:179
+struct _HasToAddress<_Pointer,
+    decltype((void)&pointer_traits<_Pointer>::to_address)
+> : true_type {};
----------------
ldionne wrote:
> I think you need to call `pointer_traits<_Pointer>::to_address(...)`, otherwise it could be a function taking no arguments, or an overloaded function (and we can't take the address of those), etc.
> 
> It could even be a static data member.
Thanks, good point. This is also consistent with the check below in `__to_address_helper`.


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/contiguous_iterator.compile.pass.cpp:237-238
+    self operator+(difference_type n) const;
+    template <bool B1, bool B2>
+    friend no_operator_arrow<B1, B2> operator+(difference_type n, no_operator_arrow<B1, B2> x);
+
----------------
huixie90 wrote:
> does it work without `template <bool B1, bool B2>` and just use `no_operator_arrow`
This is to prevent a GCC warning ("friend declaration declares a non-template function"). Added a comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130835