[libcxx-commits] [PATCH] D117714: [libc++] Implement LWG3549: view_interface need not inherit from view_base

[Joe Loser via Phabricator via libcxx-commits]

jloser marked an inline comment as done.
jloser added inline comments.


================
Comment at: libcxx/include/__ranges/enable_view.h:36
 template <class _Tp>
-inline constexpr bool enable_view = derived_from<_Tp, view_base>;
+inline constexpr bool enable_view = derived_from<_Tp, view_base> || decltype(__inherits_from_view_interface(static_cast<remove_cvref_t<_Tp>*>(nullptr)))::value;
 
----------------
Quuxplusone wrote:
> jloser wrote:
> > Quuxplusone wrote:
> > > Quuxplusone wrote:
> > > > Please add an ADL-proofing regression test that will catch the lack of qualification on `__inherits_from_view_interface`; and then change it to `_VSTD::__inherits_from_view_interface` so that the test case passes. (Grep for `Holder<Incomplete>` to see our traditional ADL trap.)
> > > FYI, you don't need (and therefore shouldn't have) qualification on non-functions. One level of qualification is needed purely to turn off ADL, and for no other purpose. So: `ranges::__derived_from_view_interface_helper(...)`, `__is_derived_from_view_interface<...>`.
> > > However, this //still// feels crazy complicated. Why isn't this just the-thing-I-posted-yesterday with the names uglified and ADL-proofed? Why does it need inline constexpr variables //and// concepts //and// partial specializations //and// the-thing-I-said?
> > We can fix up the qualifications, no problem.
> > 
> > It does feel more complicated than it needs to be - I'll play with it some more maybe later tonight. The thing we need to jump through some hoops for is to avoid `static_cast<T>(nullptr)` being a hard error (which is the case for when privately inheriting from `view_interface`). In this case, the number of hoops is more than (you or I) like.
> Ah, I do now see the problem with private inheritance: that private derived-to-base conversion is //visible// but not //accessible//. Still, we know what to do about that; see `enable_shared_from_this`, https://github.com/llvm/llvm-project/blob/main/libcxx/include/__memory/shared_ptr.h#L902-L906
> 
> Let's do this: https://godbolt.org/z/cMrE5bdWd
> ```
> template<class _Op, class _Yp>
>   requires is_convertible_v<_Op*, view_interface<_Yp>*>
> void __is_derived_from_view_interface(const _Op*, const view_interface<_Yp>*);
> 
> template<class T>
> inline constexpr bool enable_view =
>   derived_from<_Tp, view_base> ||
>   requires { _VSTD::__is_derived_from_view_interface((_Tp*)nullptr, (_Tp*)nullptr); };
> ```
> Notice that this //does// permit const-qualified object types, but //not// ref-qualified types. That's because if `X` derives directly from `view_base`, then `derived_from<const X, view_base> == enable_view<const X> == true` according to the Standard. So, analogously, if `X` derives from `view_interface<Y>`, `enable_view<const X>` should remain `true`.
> Btw, please add some tests for `X`-deriving-directly-from-`view_base`, if there aren't already some. If they //did// exist, then consistency between those tests and these should have demonstrated what we needed to do about `const` from the get-go; which is why I'm assuming there were no such tests. (Serendipitously, I just wrote a relevant blog post this morning! ;) https://quuxplusone.github.io/blog/2022/01/20/the-economists-100-bill/ )
Exactly spot on - it's a visible-but-not-accessible thing which is why we can't do the original simple thing you suggested. I wasn't trying to introduce complexity without justice as your blog post discusses ;).

Just adopted your suggestion to clean up the implementation but do the right thing for const and ref-qualified types. 

Opened https://reviews.llvm.org/D117918 for the additional `enable_view` tests unrelated to this change. We already had some tests for inheriting from `enable_view`, but they didn't show the interesting properties with const or reference qualified types. This fixes that so that this PR follows a similar exhaustive pattern of testing all the const/ref-qualified types.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117714