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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added inline comments.


================
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>;
+};
----------------
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>;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112665