Page MenuHomePhabricator

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

Authored by var-const on Jul 31 2022, 2:10 AM.

Details

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.

Diff Detail

Event Timeline

var-const created this revision.Jul 31 2022, 2:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 31 2022, 2:10 AM
var-const requested review of this revision.Jul 31 2022, 2:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 31 2022, 2:10 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
var-const added inline comments.Jul 31 2022, 2:12 AM
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 updated this revision to Diff 448900.Jul 31 2022, 6:24 PM

Fix the hard error during SFINAE

var-const added inline comments.Jul 31 2022, 6:27 PM
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 edited the summary of this revision. (Show Details)Jul 31 2022, 9:51 PM
var-const updated this revision to Diff 448916.Jul 31 2022, 9:51 PM

Fix the CI (GCC).

var-const updated this revision to Diff 448932.Aug 1 2022, 12:28 AM

Fix GCC again.

ldionne requested changes to this revision.Aug 2 2022, 3:48 PM

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.

This revision now requires changes to proceed.Aug 2 2022, 3:48 PM
huixie90 accepted this revision as: huixie90.Aug 4 2022, 12:15 AM
huixie90 added a subscriber: 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 updated this revision to Diff 449929.Aug 4 2022, 4:21 AM
var-const marked 4 inline comments as done.

Address feedback.

var-const added inline comments.Aug 4 2022, 4:22 AM
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.

ldionne accepted this revision.Aug 4 2022, 5:44 AM
This revision is now accepted and ready to land.Aug 4 2022, 5:44 AM