[libcxx-commits] [PATCH] D115838: [libc++] [ranges] Remove the static_assert from ranges::begin and ranges::end.

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__ranges/access.h:65
+      requires is_array_v<remove_cv_t<_Tp>>
+    [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp& __t) const noexcept
+    {
----------------
ldionne wrote:
> Could we add another overload like this:
> 
> ```
> template <class _Tp>
>       requires is_array_v<remove_cv_t<_Tp>> && !__is_complete<iter_value_t<_Tp>>
> void operator()(_Tp&&) = delete;
> ```
> 
> of something of that sort? Otherwise, why don't we simply add the constraint `__is_complete` to this overload?
Here, I'm intentionally permitting `ranges::begin(x)` to return `x` when `x` is an array of incomplete type, because that seems strictly more useful than any other behavior (and it's IFNDR so we get to pick our behavior). Notice that if the type of `*x` //becomes// complete, later in the TU, we haven't violated the ODR yet. If we did what you suggest above, and made the behavior of this function dependent on the completeness of `*x`, then we //would// be violating the ODR.

See my comment on `ranges::end` also; that rationale also applies here. But in the `ranges::end` case, we don't have the option to choose "Don't violate the ODR"; there we //must// simply try to minimize the collateral damage.


================
Comment at: libcxx/include/__ranges/access.h:126
+    [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp (&__t)[_Np]) const noexcept
+      requires (sizeof(*__t) != 0)
+    {
----------------
ldionne wrote:
> Why not use `__is_complete`? The name conveys more information than this expression IMO.
I think it's valuable for us to stay away from any named entity that might be cached by the compiler — no type trait, no concept. The //only// thing we have to commit to (and maybe get cached) on this line is the well-formedness of `ranges::end`. We don't want this to have spooky effects on the well-formedness of other unrelated things as compilation of this TU proceeds (which is what would happen if we shared a dependency on `__is_complete` or whatever).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115838