[libc++] Implement LWG3549: view_interface need not inherit from view_base

[jloser]

Summary

Implement LWG3549 by making view_interface not inherit from view_base. Types
are still views if they have a public and unambiguous derivation from
view_interface, so adjust the enable_view machinery as such to account for
that.

[• Quuxplusone]

@jloser: I believe the intended implementation technique here is

template<class U> std::true_type f(std::view_interface<U>*);
std::false_type f(...);
template<class T> using the_thing = decltype(f( (T*)nullptr ));

(with all the obvious uglifications etc). Check out what we do for enable_shared_from_this and/or grep for any other places in the Standard that use that same "unique public base class" phrasing. (I have not checked, myself.)

[jloser]

Fix public and unambiguous inheritance with view_interface in enable_view definition. This should fix CI!

[jloser]

In D117714#3256022, @Quuxplusone wrote:

@jloser: I believe the intended implementation technique here is

template<class U> std::true_type f(std::view_interface<U>*);
std::false_type f(...);
template<class T> using the_thing = decltype(f( (T*)nullptr ));

(with all the obvious uglifications etc). Check out what we do for enable_shared_from_this and/or grep for any other places in the Standard that use that same "unique public base class" phrasing. (I have not checked, myself.)

Yeah, that's about what I'd expect. I just updated the definition of enable_view to reflect this which should pass CI.

[• Quuxplusone]

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)

libcxx/include/__ranges/enable_view.h

38–39

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

libcxx/include/__ranges/view_interface.h

37–38

Pre-existing: I bet we reinvent this helper all over the place. We should centralize it in <type_traits>.
(No action required here.)

75

Pre-existing: The repeated typo _D2 for _D2& suggests to me that our test coverage for view_interface is somewhat insufficient.

[jloser]

libcxx/include/__ranges/view_interface.h

37–38

Yeah, I had the same thought. It is reinvented or taken as a direct function parameter (such as in span and iota_view). Might be worth cleaning up, but I don't feel strongly.

75

Agreed. I can look into it as a follow-up.

[Mordante]

Thanks for working on this.

libcxx/include/__ranges/enable_view.h

33

Please use is_derived instead of inherits, the former matches the naming in the Standard is-derived-from-view-interface.

libcxx/test/std/ranges/range.utility/view.interface/view.interface.pass.cpp

21

I miss new tests that validate the constrains set in LWG 3549
"For a type T, is-derived-from-view-interface<T> is true if and only if T has exactly one public base class view_interface<U> for some type U and T has no base classes of type view_interface<V> for any other type V."

For example
struct NotAValidViewInterace : std::ranges::view_interface<InputRange>, std::ranges::view_interface<MoveOnlyForwardRange>{};

[jloser]

Add some more tests. Still missing ADL-proofing test Arthur would like

[jloser]

libcxx/test/std/ranges/range.utility/view.interface/view.interface.pass.cpp

21

Added some tests both here and in enable_view.compile.pass.cpp.

[jloser]

Add ADL-proofing test
Add private inheritance test showing the static_cast approach hard errors. Rework implementatin to be clever and rely on lambdas in unevaluated contexts. If this doesn't work yet on CI, we can delegate to a helper functin template.

[• Quuxplusone]

Rework implementation to be clever

Please don't, though. There was nothing wrong with the original simple code; complexifying it just makes it harder to reason about (and more fragile/less portable, as your comment seems to anticipate).

[jloser]

Less clever but support private inheritance still. GCC11 doesn't like lambdas in unevaluated contexts yet, anyway.

[jloser]

Uncomment the other two const and const ref private inheritance tests for enable_view. Oops.

[• Quuxplusone]

libcxx/include/__ranges/enable_view.h

38–39

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?

libcxx/test/std/ranges/range.req/range.view/enable_view.compile.pass.cpp

50–53 ↗

(On Diff #401772)

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>.

[jloser]

Remove const and const ref tests for enable_view. The standard expects cv-unqual types.

[jloser]

enable_view<void> test

[jloser]

libcxx/include/__ranges/enable_view.h

38–39

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.

[jloser]

Simplify is-derived-from-view-interface by using a concept instead

[jloser]

Make enable_view<const V> and enable_view<const V&> and friends false. No support for it in the standard, despite implementation divergence.

[jloser]

libcxx/test/std/ranges/range.req/range.view/enable_view.compile.pass.cpp

50–53 ↗

(On Diff #401772)

I just wrote a simple implementation to make enable_view<const V1> and enable_view<const V1&> and friends false. There isn't support for it in the Standard, despite the fact that there is implementation divergence.

[• Quuxplusone]

libcxx/include/__ranges/enable_view.h

38–39

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

[jloser]

Fix hacky implementation for guarding against private inheritance case per Arthur's suggestion

[jloser]

Remove old implementation, oops.

[jloser]

libcxx/include/__ranges/enable_view.h

38–39

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.

[jloser]

Rebase now that https://reviews.llvm.org/D117918 landed

[• Quuxplusone]

LGTM! Please wait for @ldionne or @Mordante to take a look too.

libcxx/include/__ranges/view_interface.h

20–21

Pre-existing but perhaps relevant: This line can be eliminated.

libcxx/test/std/ranges/range.utility/view.interface/view.interface.pass.cpp

82–84

Please do something to fix the long line. I suggest s/MultipleInheritanceViewInterface/MI/g.
(Also, wasn't this supposed to be landed as part of D117918? Too late now, anyway.)

310

Arguably this should be LIBCPP_STATIC_ASSERT since this behavior is not mandated; but every other vendor has already beaten us to implement this, so I think a plain static_assert is fine.

[Mordante]

LGTM, please fix the line-size comment before committing.

libcxx/test/std/ranges/range.utility/view.interface/view.interface.pass.cpp

82–84

+1

[jloser]

Fix line length comment in v iew.interface.pass.cpp
Remove <__ranges/enable_view.h> include from view_interface.h

If CI passes, I'll ship this later tonight

[jloser]

libcxx/include/__ranges/view_interface.h

20–21

Removed it. Shouldn't cause any issues from local testing, but we'll let CI run to make sure before I commit this.

libcxx/docs/Status/Cxx2bIssues.csv

 `3539 <https://wg21.link/LWG3539>`__,"``format_to`` must not copy models of ``output_iterator<const charT&>``","June 2021","","","|format|"
 `3540 <https://wg21.link/LWG3540>`__,"§[format.arg] There should be no const in ``basic_format_arg(const T* p)``","June 2021","","","|format|"
 `3541 <https://wg21.link/LWG3541>`__,"``indirectly_readable_traits`` should be SFINAE-friendly for all types","June 2021","","","|ranges|"
 `3542 <https://wg21.link/LWG3542>`__,"``basic_format_arg`` mishandles ``basic_string_view`` with custom traits","June 2021","|Complete|","14.0","|format|"
 `3543 <https://wg21.link/LWG3543>`__,"Definition of when ``counted_iterators`` refer to the same sequence isn't quite right","June 2021","","","|ranges|"
 `3544 <https://wg21.link/LWG3544>`__,"``format-arg-store::args`` is unintentionally not exposition-only","June 2021","","","|format|"
 `3546 <https://wg21.link/LWG3546>`__,"``common_iterator``'s postfix-proxy is not quite right","June 2021","","","|ranges|"
 `3548 <https://wg21.link/LWG3548>`__,"``shared_ptr`` construction from ``unique_ptr`` should move (not copy) the deleter","June 2021","",""
-`3549 <https://wg21.link/LWG3549>`__,"``view_interface`` is overspecified to derive from ``view_base``","June 2021","","","|ranges|"
+`3549 <https://wg21.link/LWG3549>`__,"``view_interface`` is overspecified to derive from ``view_base``","June 2021","|Complete|","14.0","|ranges|"
 `3551 <https://wg21.link/LWG3551>`__,"``borrowed_{iterator,subrange}_t`` are overspecified","June 2021","","","|ranges|"
 `3552 <https://wg21.link/LWG3552>`__,"Parallel specialized memory algorithms should require forward iterators","June 2021","",""
 `3553 <https://wg21.link/LWG3553>`__,"Useless constraint in ``split_view::outer-iterator::value_type::begin()``","June 2021","","","|ranges|"
 `3555 <https://wg21.link/LWG3555>`__,"``{transform,elements}_view::iterator::iterator_concept`` should consider const-qualification of the underlying range","June 2021","","","|ranges|"
 "","","","",""
 `2191 <https://wg21.link/LWG2191>`__,"Incorrect specification of ``match_results(match_results&&)``","October 2021","",""
 `2381 <https://wg21.link/LWG2381>`__,"Inconsistency in parsing floating point numbers","October 2021","",""
 `2762 <https://wg21.link/LWG2762>`__,"``unique_ptr operator*()`` should be ``noexcept``","October 2021","",""

 `3590 <https://wg21.link/LWG3590>`__,"``split_view::base() const &`` is overconstrained","October 2021","","","|ranges|"
 `3591 <https://wg21.link/LWG3591>`__,"``lazy_split_view<input_view>::inner-iterator::base() &&`` invalidates outer iterators","October 2021","","","|ranges|"
 `3592 <https://wg21.link/LWG3592>`__,"``lazy_split_view`` needs to check the simpleness of Pattern","October 2021","","","|ranges|"
 `3593 <https://wg21.link/LWG3593>`__,"Several iterators' ``base() const &`` and ``lazy_split_view::outer-iterator::value_type::end()`` missing ``noexcept``","October 2021","","","|ranges|"
 `3595 <https://wg21.link/LWG3595>`__,"Exposition-only classes proxy and postfix-proxy for ``common_iterator`` should be fully ``constexpr``","October 2021","","","|ranges|"
 "","","","",""
 `3645 <https://wg21.link/LWG3645>`__,"``resize_and_overwrite`` is overspecified to call its callback with lvalues", "Not voted in","|Complete|","14.0",""
 "","","","",""
 No newline at end of file

libcxx/include/__ranges/enable_view.h

 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef _LIBCPP___RANGES_ENABLE_VIEW_H
 #define _LIBCPP___RANGES_ENABLE_VIEW_H
 
 #include <__config>
 #include <concepts>
+#include <type_traits>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #pragma GCC system_header
 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 #if !defined(_LIBCPP_HAS_NO_RANGES)
 
 namespace ranges {
 
 struct view_base { };
 
+template<class _Derived>
+  requires is_class_v<_Derived> && same_as<_Derived, remove_cv_t<_Derived>>
+class view_interface;
+
+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 _Tp>
-inline constexpr bool enable_view = derived_from<_Tp, view_base>;
+inline constexpr bool enable_view = derived_from<_Tp, view_base> ||
+  requires { ranges::__is_derived_from_view_interface((_Tp*)nullptr, (_Tp*)nullptr); };
 
 } // end namespace ranges
 
 #endif // !_LIBCPP_HAS_NO_RANGES
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif // _LIBCPP___RANGES_ENABLE_VIEW_H

libcxx/include/__ranges/view_interface.h

 #include <__config>
 #include <__debug>
 #include <__iterator/concepts.h>
 #include <__iterator/iterator_traits.h>
 #include <__iterator/prev.h>
 #include <__memory/pointer_traits.h>
 #include <__ranges/access.h>
 #include <__ranges/concepts.h>
 #include <__ranges/empty.h>
-#include <__ranges/enable_view.h>
 #include <concepts>
 #include <type_traits>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #pragma GCC system_header
 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 #if !defined(_LIBCPP_HAS_NO_RANGES)
 
 namespace ranges {
 
 template<class _Tp>
 concept __can_empty = requires(_Tp __t) { ranges::empty(__t); };
 
 template<class _Tp>
 void __implicitly_convert_to(type_identity_t<_Tp>) noexcept;
 
 template<class _Derived>
   requires is_class_v<_Derived> && same_as<_Derived, remove_cv_t<_Derived>>
-class view_interface : public view_base {
+class view_interface {
   _LIBCPP_HIDE_FROM_ABI
   constexpr _Derived& __derived() noexcept {
     static_assert(sizeof(_Derived) && derived_from<_Derived, view_interface> && view<_Derived>);
     return static_cast<_Derived&>(*this);
   }
 
   _LIBCPP_HIDE_FROM_ABI
   constexpr _Derived const& __derived() const noexcept {
[[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr bool empty() const
     requires forward_range<const _D2>
   {
     return ranges::begin(__derived()) == ranges::end(__derived());
   }
 
   template<class _D2 = _Derived>
   _LIBCPP_HIDE_FROM_ABI
   constexpr explicit operator bool()
     noexcept(noexcept(ranges::empty(declval<_D2>())))
     requires __can_empty<_D2>
   {
     return !ranges::empty(__derived());
   }
 
   template<class _D2 = _Derived>
   _LIBCPP_HIDE_FROM_ABI
   constexpr explicit operator bool() const

libcxx/test/std/ranges/range.utility/view.interface/view.interface.pass.cpp

 // template<class D>
 //   requires is_class_v<D> && same_as<D, remove_cv_t<D>>
 // class view_interface;
 
 #include <ranges>
 
 #include <cassert>
 #include "test_macros.h"
 #include "test_iterators.h"
 
 template<class T>
 concept ValidViewInterfaceType = requires { typename std::ranges::view_interface<T>; };
 
 struct Empty { };
 
 static_assert(!ValidViewInterfaceType<void>);
 static_assert(!ValidViewInterfaceType<void*>);
struct MoveOnlyForwardRange : std::ranges::view_interface<MoveOnlyForwardRange> {
   MoveOnlyForwardRange(MoveOnlyForwardRange &&) = default;
   MoveOnlyForwardRange& operator=(MoveOnlyForwardRange &&) = default;
   MoveOnlyForwardRange() = default;
   constexpr ForwardIter begin() const { return ForwardIter(const_cast<int*>(buff)); }
   constexpr ForwardIter end() const { return ForwardIter(const_cast<int*>(buff) + 8); }
 };
 static_assert(std::ranges::view<MoveOnlyForwardRange>);
 
+struct MI : std::ranges::view_interface<InputRange>,
+            std::ranges::view_interface<MoveOnlyForwardRange> {
+};
+static_assert(!std::ranges::view<MI>);
+
 struct EmptyIsTrue : std::ranges::view_interface<EmptyIsTrue> {
   int buff[8] = {0, 1, 2, 3, 4, 5, 6, 7};
   constexpr ForwardIter begin() const { return ForwardIter(const_cast<int*>(buff)); }
   constexpr ForwardIter end() const { return ForwardIter(const_cast<int*>(buff) + 8); }
   constexpr bool empty() const { return true; }
 };
 static_assert(std::ranges::view<EmptyIsTrue>);
 
constexpr bool testFrontBack() {
   assert(randomAccess.back() == 7);
   assert(static_cast<RARange const&>(randomAccess).back() == 7);
   randomAccess.back() = 2;
   assert(randomAccess.back() == 2);
 
   return true;
 }
 
+struct V1 : std::ranges::view_interface<V1> { };
+struct V2 : std::ranges::view_interface<V2> { V1 base_; };
+static_assert(sizeof(V2) == sizeof(V1));
+
 int main(int, char**) {
   testEmpty();
   static_assert(testEmpty());
 
   testData();
   static_assert(testData());
 
   testSize();