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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

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!



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


================
Comment at: libcxx/include/__memory/pointer_traits.h:179
+struct _HasToAddress<_Pointer,
+    decltype((void)&pointer_traits<_Pointer>::to_address)
+> : true_type {};
----------------
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.


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/contiguous_iterator.compile.pass.cpp:252-253
+
+static_assert(std::contiguous_iterator<no_operator_arrow</*DisableArrow=*/false>>);
+static_assert(!std::contiguous_iterator<no_operator_arrow</*DisableArrow=*/true>>);
+static_assert(std::contiguous_iterator<no_operator_arrow</*DisableArrow=*/true, /*DisableToAddress=*/false>>);
----------------
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.


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/contiguous_iterator.compile.pass.cpp:226
+    reference operator*() const;
+    pointer operator->() const requires (!DisableArrow);
+    auto operator<=>(const self&) const = default;
----------------
var-const wrote:
> 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).
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130835