[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>;
+};
----------------
jloser wrote:
> jloser wrote:
> > 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.
> Agree regarding order and checking completeness first. I removed the concept in the latest iteration (and also don't check for completeness). What do you think, @cjdb (concept vs inline, etc)?
> What do you think, @cjdb (concept vs inline, etc)?

I like names that express intent.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112665