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

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

Quuxplusone requested changes to this revision.
Quuxplusone added inline comments.
This revision now requires changes to proceed.


================
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:
> 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?


================
Comment at: libcxx/test/std/ranges/range.req/range.view/enable_view.compile.pass.cpp:50-53
+struct V1 : std::ranges::view_interface<V1> {};
+static_assert(std::ranges::enable_view<V1>);
+static_assert(std::ranges::enable_view<const V1>);
+static_assert(std::ranges::enable_view<const V1&>);
----------------
Let's also test
```
struct V3 : std::ranges::view_interface<V1> {};
static_assert(std::ranges::enable_view<V3>);
static_assert(!std::ranges::enable_view<V3&>);
static_assert(!std::ranges::enable_view<V3&&>);
static_assert(!std::ranges::enable_view<const V3>);
static_assert(!std::ranges::enable_view<const V3&>);
static_assert(!std::ranges::enable_view<const V3&&>);
```
I'm actually confused that `enable_view<const V1>` and `<const V1&>` are true; I don't see any textual support for that in http://eel.is/c++draft/range.view . Can you see why that's happening, and fix it if need be?

Since our implementation is messing around with pointer conversions, let's also sanity-check that `!enable_view<void>`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117714