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

[Joe Loser via Phabricator via libcxx-commits]

jloser added a comment.

In D112665#3091724 <https://reviews.llvm.org/D112665#3091724>, @Quuxplusone wrote:

> LGTM % (significant) comments, but I'm sure there will be some discussion.





================
Comment at: libcxx/include/__ranges/view_interface.h:45-50
+template<class _Tp>
+concept __valid_view_for_view_interface = requires {
+  sizeof(_Tp);
+  derived_from<_Tp, view_interface<_Tp>>;
+  view<_Tp>;
+};
----------------
cjdb wrote:
> Quuxplusone wrote:
> > Flashbacks to "Concepts As She Is Spoke"! :) Remember that `view<_Tp>` is //always// well-formed. Sometimes its runtime boolean //value// will be "false," but it's always //well-formed// (and has type `bool`). What you meant was
> > ```
> > template<class _Tp>
> > concept __valid_view_for_view_interface =
> >   view<_Tp> && derived_from<_Tp, view_interface<_Tp>> && requires { sizeof(_Tp); };
> > ```
> > 
> > However, I really don't think this should be a `concept`, because it depends on a property (completeness) that changes over the lifetime of the TU. I think you should just inline this into the one place it's used: that `static_assert`.
> > 
> > Collateral bonuses:
> > - no need to forward-declare `view_interface` on line 43
> > - probably no need to include the second argument to `static_assert`
> > - optionally (and I'd prefer it), use the injected-class-name `derived_from<_Derived, view_interface>` instead of `derived_from<_Derived, view_interface<_Derived>>`
> The original order is important: we want to check for completeness first, //then// the cheapest independent check, and so on. But yes, only the former should be in the requires clause.
> 
> ```
> template<class _Tp>
> concept __valid_view_for_view_interface =
>   requires { sizeof(_Tp); } &&
>   derived_from<_Tp, view_interface<_Tp>> &&
>   view<_Tp>;
> ```
Yikes, you're right. I meant what you typed. 

I removed the concept (and completeness check) in favor of inlining it as you suggested.


================
Comment at: libcxx/include/__ranges/view_interface.h:65
     return static_cast<_Derived const&>(*this);
   }
 
----------------
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>> {
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112665