This is an archive of the discontinued LLVM Phabricator instance.

[libc++] No longer support ranges::begin(x) when x is an array of incomplete type.
ClosedPublic

Authored by Quuxplusone on Feb 3 2022, 6:46 PM.

Details

Summary

var-const points out that ranges::begin is (non-normatively but explicitly) always supposed to return a std::input_or_output_iterator, and Incomplete* is not a std::input_or_output_iterator because it has no operator++. Therefore, we should never return Incomplete* from ranges::begin(x), even when x is Incomplete(&)[]. Instead, just SFINAE away.

(The note in question: https://eel.is/c++draft/range.access.begin#4 "Whenever ranges​::​begin(E) is a valid expression, its type models input_­or_­output_­iterator.")

Diff Detail

Event Timeline

Quuxplusone requested review of this revision.Feb 3 2022, 6:46 PM
Quuxplusone created this revision.
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptFeb 3 2022, 6:46 PM
Quuxplusone added inline comments.Feb 3 2022, 6:49 PM
libcxx/include/__ranges/access.h
73

The use of _Tp (&__t)[] here instead of is_array_v is both for symmetry with ranges::end, and because we need to make sure that we never try to evaluate *__t before we're absolutely sure that __t is an array type (because if it's a class type, that'll do ADL and we don't want ADL).
__t + 0 could be just __t AFAIK, but the standard says __t + 0 and I can't see any way for that to hurt anything.

var-const accepted this revision as: var-const.Feb 3 2022, 8:35 PM

Thanks!

var-const added inline comments.Feb 3 2022, 10:00 PM
libcxx/include/__ranges/access.h
62

Question: end only defines the case where the array size is known. Does the case where the size is unknown need to be handled by end, or does it not apply in that case?

var-const added inline comments.Feb 3 2022, 10:05 PM
libcxx/include/__ranges/access.h
73

Question: why do we need to do sizeof(*__t) rather than sizeof(_Tp)?

philnik added inline comments.Feb 4 2022, 3:44 AM
libcxx/include/__ranges/access.h
62

Couldn't we just say input_or_output_iterator auto operator()?

Quuxplusone marked 3 inline comments as done.Feb 4 2022, 8:31 AM
Quuxplusone added inline comments.
libcxx/include/__ranges/access.h
62

When the array size is unknown, i.e. we're asking for the ranges::end of an int[], well, we have no idea where that end would be! :) So ranges::end(int[]) is ill-formed https://eel.is/c++draft/range.access.end#2.3 (note: not even IFNDR).

62

Couldn't we just say input_or_output_iterator auto operator()?

That would have caught this bug, in the sense that it would have caused hard errors in some of our tests. (Remember a constrained return type is basically a static_assert.) However, as a matter of QoI I'd still prefer not to pay for that constraint check on every single instantiation of ranges::begin.
I did add the constrained return type locally and confirmed that all our tests pass now.

The other awkward thing about constrained auto return types is that, AFAICT, it is impossible to use both a constrained return type and a trailing -> decltype(...) return type, which means that we wouldn't be able to use constrained return types at all if we moved to a "write it three times" style like we currently do in ranges::cbegin, or a "write it four times" style like I propose in D115607.

FYI, ranges::end has a similar "kinda normative note," that its return type always satisfies sentinel_for<iterator_t<_Tp>> https://eel.is/c++draft/range.access.end#4

73

Good point, will fix. On the left-hand side, we needed *__t because _Tp& __t referred to the whole array. Now that _Tp refers to the element type, sizeof(_Tp) should be fine.

Quuxplusone marked 3 inline comments as done.

sizeof(_Tp)

philnik accepted this revision.Feb 4 2022, 12:45 PM
This revision is now accepted and ready to land.Feb 4 2022, 12:45 PM
ldionne accepted this revision.Feb 4 2022, 1:05 PM

This should be cherry-picked to 14.x

libcxx/include/__ranges/access.h