[libcxx-commits] [PATCH] D112665: [libc++] Ensure valid view for view_interface template parameter

[Joe Loser via Phabricator via libcxx-commits]

jloser added inline comments.


================
Comment at: libcxx/include/__ranges/view_interface.h:65
     return static_cast<_Derived const&>(*this);
   }
 
----------------
Quuxplusone wrote:
> jloser wrote:
> > Quuxplusone wrote:
> > > > We intentionally do it as a static_assert instead of a requires clause to avoid hard erroring in some cases.
> > > 
> > > And also because this is an implementation detail: not libc++ policy but certainly my own policy, is that we can cruft up the //public// API (with `noexcept`, `requires`, `[[nodiscard]]`, etc) when the Standard forces us to, but for the //private// details, "behind the firewall" as it were, we should prefer simple and fast and generic.
> > > 
> > > > These cases include checking a particular concept that delegates to a type trait that hard errors when passed an incomplete type.
> > > 
> > > I don't understand this sentence at all. Are you thinking of a particular concept in particular? 😛 If you can craft an example program that fails when `__derived()` is constrained but succeeds when `__derived()` is unconstrained, then please add it as a test case. (Which will also forestall future relitigation of this issue, because then we'll have a test case that fails if someone tries it.)
> > This may just be an obvious error on my half, so I recommend you try it out. But if you take the equivalent boolean expression in the `static_assert` and naively turn it into a `requires` for the `__derived()` member functions, you'll get a hard error in the `view.interface.pass` existing test. I think this makes sense to me since it's hard erroring in evaluating `derived_from` concept with an incomplete type. An example error is:
> > 
> > ```
> > /Users/joe/dev_local/llvm-project/build/include/c++/v1/type_traits:1661:59: error: incomplete type 'BoolConvertibleComparison<false>' used in type trait expression
> >     : public integral_constant<bool, __is_base_of(_Bp, _Dp)> {};
> >                                                           ^
> > /Users/joe/dev_local/llvm-project/build/include/c++/v1/type_traits:1665:38: note: in instantiation of template class 'std::is_base_of<std::ranges::view_interface<BoolConvertibleComparison<false>>, BoolConvertibleComparison<false>>' requested here
> > inline constexpr bool is_base_of_v = is_base_of<_Bp, _Dp>::value;
> >                                      ^
> > /Users/joe/dev_local/llvm-project/build/include/c++/v1/__concepts/derived_from.h:27:3: note: in instantiation of variable template specialization 'std::is_base_of_v<std::ranges::view_interface<BoolConvertibleComparison<false>>, BoolConvertibleComparison<false>>' requested here
> >   is_base_of_v<_Bp, _Dp> &&
> >   ^
> > /Users/joe/dev_local/llvm-project/build/include/c++/v1/__concepts/derived_from.h:27:3: note: while substituting template arguments into constraint expression here
> >   is_base_of_v<_Bp, _Dp> &&
> >   ^~~~~~~~~~~~~~~~~~~~~~
> > /Users/joe/dev_local/llvm-project/build/include/c++/v1/__ranges/view_interface.h:45:71: note: while checking the satisfaction of concept 'derived_from<BoolConvertibleComparison<false>, std::ranges::view_interface<BoolConvertibleComparison<false>>>' requested here
> >   constexpr _Derived& __derived() noexcept requires view<_Derived> && derived_from<_Derived, view_interface> {
> >                                                                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > /Users/joe/dev_local/llvm-project/libcxx/test/std/ranges/range.utility/view.interface/view.interface.pass.cpp:127:36: note: in instantiation of template class 'std::ranges::view_interface<BoolConvertibleComparison<false>>' requested here
> > struct BoolConvertibleComparison : std::ranges::view_interface<BoolConvertibleComparison<IsNoexcept>> {
> > ```
> Ah. IIUC (which I still may not), that'll be fixed by @cjdb's recommendation to put `requires { sizeof(_Tp); }` //first// and `derived_from<...>` afterward.
> If you do that, //then// are there any failures to avoid?
It's still an issue. With the concept (even with it checking for completeness using `sizeof` first), when you convert the use from `static_assert` to `requires`, you will get a hard error for incomplete types when the type is passed to `sizeof` when we're checking concept constraints. This is a bit unintuitive to me. Does this make sense to you or @cjdb?  See the updated commit message for an example error.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112665