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

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

Quuxplusone 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;
 
----------------
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/ )


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117714