This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by jloser on Jan 19 2022, 12:27 PM.

Details

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.

Diff Detail

Event Timeline

jloser requested review of this revision.Jan 19 2022, 12:27 PM
jloser created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 19 2022, 12:27 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone added a comment.EditedJan 19 2022, 12:51 PM

@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 updated this revision to Diff 401441.Jan 19 2022, 4:32 PM

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

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

jloser edited the summary of this revision. (Show Details)Jan 19 2022, 4:34 PM
jloser retitled this revision from [WIP][libc++] Implement LWG3549: view_interface need not inherit from view_base to [libc++] Implement LWG3549: view_interface need not inherit from view_base.

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 added inline comments.Jan 19 2022, 6:29 PM
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.

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 updated this revision to Diff 401731.Jan 20 2022, 12:07 PM

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

jloser marked 2 inline comments as done.Jan 20 2022, 12:10 PM
jloser added inline comments.
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 updated this revision to Diff 401744.Jan 20 2022, 12:30 PM
jloser marked an inline comment as done.

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.

jloser marked an inline comment as done.Jan 20 2022, 12:31 PM

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 updated this revision to Diff 401758.Jan 20 2022, 1:09 PM

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

jloser updated this revision to Diff 401772.Jan 20 2022, 1:44 PM

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

Quuxplusone requested changes to this revision.Jan 20 2022, 2:34 PM
Quuxplusone added inline comments.
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>.

This revision now requires changes to proceed.Jan 20 2022, 2:34 PM
jloser updated this revision to Diff 401798.Jan 20 2022, 3:19 PM

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

jloser updated this revision to Diff 401799.Jan 20 2022, 3:20 PM

enable_view<void> test

jloser added inline comments.Jan 20 2022, 3:24 PM
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 updated this revision to Diff 401826.Jan 20 2022, 5:42 PM

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

jloser updated this revision to Diff 401830.Jan 20 2022, 6:05 PM
jloser marked 3 inline comments as done.

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

jloser marked an inline comment as done.Jan 20 2022, 6:13 PM
jloser added inline comments.
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.

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 updated this revision to Diff 402064.Jan 21 2022, 12:10 PM
jloser marked an inline comment as done.

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

jloser updated this revision to Diff 402067.Jan 21 2022, 12:13 PM

Remove old implementation, oops.

jloser marked an inline comment as done.Jan 21 2022, 12:19 PM
jloser added inline comments.
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 updated this revision to Diff 402136.Jan 21 2022, 4:48 PM
jloser marked an inline comment as done.
jloser edited the summary of this revision. (Show Details)

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

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 accepted this revision.Jan 22 2022, 8:15 AM

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

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

+1

This revision is now accepted and ready to land.Jan 22 2022, 8:15 AM
jloser updated this revision to Diff 402274.Jan 22 2022, 5:10 PM

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 marked 3 inline comments as done.Jan 22 2022, 5:11 PM
jloser added inline comments.
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.