[libcxx-commits] [PATCH] D118963: [libc++] No longer support ranges::begin(x) when x is an array of incomplete type.

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone marked 3 inline comments as done.
Quuxplusone added inline comments.


================
Comment at: libcxx/include/__ranges/access.h:62
     template <class _Tp>
-      requires is_array_v<remove_cv_t<_Tp>>
-    [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp& __t) const noexcept
+    [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp (&__t)[]) const noexcept
+      requires (sizeof(*__t) != 0)  // Disallow incomplete element types.
----------------
philnik wrote:
> var-const wrote:
> > 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?
> Couldn't we just say `input_or_output_iterator auto operator()`?
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).


================
Comment at: libcxx/include/__ranges/access.h:62
     template <class _Tp>
-      requires is_array_v<remove_cv_t<_Tp>>
-    [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp& __t) const noexcept
+    [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp (&__t)[]) const noexcept
+      requires (sizeof(*__t) != 0)  // Disallow incomplete element types.
----------------
Quuxplusone wrote:
> philnik wrote:
> > var-const wrote:
> > > 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?
> > Couldn't we just say `input_or_output_iterator auto operator()`?
> 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).
> 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


================
Comment at: libcxx/include/__ranges/access.h:73
+      return __t + 0;
     }
 
----------------
var-const wrote:
> Quuxplusone wrote:
> > 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.
> Question: why do we need to do `sizeof(*__t)` rather than `sizeof(_Tp)`?
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118963