[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 a comment.

Probably you should add a libcxx/test/libcxx/ test that proves the common `view_base` base is gone. Something like

  struct V1 : view_interface<V1> { };
  struct V2 : view_interface<V2> { V1 base_; };
  static_assert(sizeof(V2) == sizeof(V1));

(check via Godbolt that this fails today, of course; and add member functions if needed but I think they're actually not needed)



================
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;
 
----------------
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.)


================
Comment at: libcxx/include/__ranges/view_interface.h:38-39
 
 template<class _Tp>
 void __implicitly_convert_to(type_identity_t<_Tp>) noexcept;
 
----------------
Pre-existing: I bet we reinvent this helper all over the place. We should centralize it in <type_traits>.
(No action required here.)


================
Comment at: libcxx/include/__ranges/view_interface.h:76
   constexpr explicit operator bool()
     noexcept(noexcept(ranges::empty(declval<_D2>())))
     requires __can_empty<_D2>
----------------
Pre-existing: The repeated typo `_D2` for `_D2&` suggests to me that our test coverage for `view_interface` is somewhat insufficient.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117714