[libc++] Fix a hard error in `contiguous_iterator<NoOperatorArrowIter>`.

[var-const]

Summary

Evaluating contiguous_iterator on an iterator that satisfies all the
constraints except the to_address constraint and doesn't have
operator-> defined results in a hard error. This is because
instantiating to_address ends up instantiating templates
dependent on the given type which might lead to a hard error even
in a SFINAE context.

[var-const]

libcxx/include/__memory/pointer_traits.h

232

I'm not sure this is the right fix, but I can't think of too many alternatives (another one is to add a constraint to contiguous_iterator for the type to have operator-> defined).

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/contiguous_iterator.compile.pass.cpp

226

What do you think of this approach? I think it could be useful for other types that follow a similar pattern (*almost* support a certain constraint).

[var-const]

Fix the hard error during SFINAE

[var-const]

libcxx/include/__memory/pointer_traits.h

197

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?

197

_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>.

[var-const]

Fix the CI (GCC).

[var-const]

Fix GCC again.

[ldionne]

This mostly LGTM, but I'd like to see it again just to see how it turns out. Please ping me when my comments have been applied. Thanks and good find!

libcxx/include/__memory/pointer_traits.h

179

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.

197

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.

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/contiguous_iterator.compile.pass.cpp

226

I like it, however I can see how it becomes hard to understand if we add too many bool template parameters. I think the fact that you add C-style comments whenever you instantiate no_operator_arrow kind of covers up for it.

252–253

I guess we should also remove the default values in the template parameter. IMO that's more verbose but also more readable, otherwise I definitely have to re-read the template declaration almost every time I see an instantiation.

[huixie90]

LGTM after Louis's comments are addressed

libcxx/include/__memory/pointer_traits.h

176

I think your fix and Louis's suggestion read better. just out of curiosity, does it work with using _Lazy and _And ?

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/contiguous_iterator.compile.pass.cpp

237–238

does it work without template <bool B1, bool B2> and just use no_operator_arrow

[var-const]

Address feedback.

[var-const]

libcxx/include/__memory/pointer_traits.h

179

Thanks, good point. This is also consistent with the check below in __to_address_helper.

197

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?

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/contiguous_iterator.compile.pass.cpp

237–238

This is to prevent a GCC warning ("friend declaration declares a non-template function"). Added a comment.

libcxx/include/__memory/pointer_traits.h

 
 template <class _Tp>
 _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
 _Tp* __to_address(_Tp* __p) _NOEXCEPT {
     static_assert(!is_function<_Tp>::value, "_Tp is a function type");
     return __p;
 }
 
+template <class _Pointer, class = void>
+struct _HasToAddress : false_type {};
+
+template <class _Pointer>
+struct _HasToAddress<_Pointer,
+    decltype((void)pointer_traits<_Pointer>::to_address(declval<const _Pointer&>()))
+> : true_type {};
+
+template <class _Pointer, class = void>
+struct _HasArrow : false_type {};
+
+template <class _Pointer>
+struct _HasArrow<_Pointer,
+    decltype((void)declval<const _Pointer&>().operator->())
+> : true_type {};
+
+template <class _Pointer>
+struct _IsFancyPointer {
+  static const bool value = _HasArrow<_Pointer>::value || _HasToAddress<_Pointer>::value;
+};
+
 // enable_if is needed here to avoid instantiating checks for fancy pointers on raw pointers
 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
 > >
 _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
 typename decay<decltype(__to_address_helper<_Pointer>::__call(declval<const _Pointer&>()))>::type
 __to_address(const _Pointer& __p) _NOEXCEPT {
     return __to_address_helper<_Pointer>::__call(__p);
 }
 
 template <class _Pointer, class>

 template <class _Tp>
 inline _LIBCPP_INLINE_VISIBILITY constexpr
 auto to_address(_Tp *__p) noexcept {
     return _VSTD::__to_address(__p);
 }
 
 template <class _Pointer>
 inline _LIBCPP_INLINE_VISIBILITY constexpr
-auto to_address(const _Pointer& __p) noexcept {
+auto to_address(const _Pointer& __p) noexcept -> decltype(std::__to_address(__p)) {
     return _VSTD::__to_address(__p);
 }
 #endif
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif // _LIBCPP___MEMORY_POINTER_TRAITS_H

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/contiguous_iterator.compile.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // UNSUPPORTED: c++03, c++11, c++14, c++17
 
 // template<class T>
 // concept contiguous_iterator;
 
 #include <iterator>
 #include <compare>
+#include <memory>
 
 #include "test_iterators.h"
 
 static_assert(!std::contiguous_iterator<cpp17_input_iterator<int*>>);
 static_assert(!std::contiguous_iterator<cpp20_input_iterator<int*>>);
 static_assert(!std::contiguous_iterator<forward_iterator<int*>>);
 static_assert(!std::contiguous_iterator<bidirectional_iterator<int*>>);
 static_assert(!std::contiguous_iterator<random_access_iterator<int*>>);
struct template_and_no_element_type {
     difference_type operator-(const self& n) const;
 
     reference operator[](difference_type n) const;
 };
 
 // Template param is used instead of element_type.
 static_assert(std::random_access_iterator<template_and_no_element_type<int>>);
 static_assert(std::contiguous_iterator<template_and_no_element_type<int>>);
+
+template <bool DisableArrow, bool DisableToAddress>
+struct no_operator_arrow {
+    typedef std::contiguous_iterator_tag    iterator_category;
+    typedef int                             value_type;
+    typedef int                             element_type;
+    typedef std::ptrdiff_t                  difference_type;
+    typedef int*                            pointer;
+    typedef int&                            reference;
+    typedef no_operator_arrow               self;
+
+    no_operator_arrow();
+
+    reference operator*() const;
+    pointer operator->() const requires (!DisableArrow);
+    auto operator<=>(const self&) const = default;
+
+    self& operator++();
+    self operator++(int);
+
+    self& operator--();
+    self operator--(int);
+
+    self& operator+=(difference_type n);
+    self operator+(difference_type n) const;
+    // Note: it's a template function to prevent a GCC warning ("friend declaration declares a non-template function").
+    template <bool B1, bool B2>
+    friend no_operator_arrow<B1, B2> operator+(difference_type n, no_operator_arrow<B1, B2> x);
+
+    self& operator-=(difference_type n);
+    self operator-(difference_type n) const;
+    difference_type operator-(const self& n) const;
+
+    reference operator[](difference_type n) const;
+};
+
+template<>
+struct std::pointer_traits<no_operator_arrow</*DisableArrow=*/true, /*DisableToAddress=*/false>> {
+  static constexpr int *to_address(const no_operator_arrow<true, false>&);
+};
+
+static_assert(std::contiguous_iterator<no_operator_arrow</*DisableArrow=*/false, /*DisableToAddress=*/true>>);
+static_assert(!std::contiguous_iterator<no_operator_arrow</*DisableArrow=*/true, /*DisableToAddress=*/true>>);
+static_assert(std::contiguous_iterator<no_operator_arrow</*DisableArrow=*/true, /*DisableToAddress=*/false>>);