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

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

Quuxplusone requested changes to this revision.
Quuxplusone added a comment.
This revision now requires changes to proceed.

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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112665