This is an archive of the discontinued LLVM Phabricator instance.

[libc++] [ranges] Implement P2415R2 owning_view

Authored by Quuxplusone on Jan 9 2022, 9:38 AM.



"What is a view?"

This was a late-breaking (Oct 2021) change to C++20.
The only thing missing from this patch is that we're supposed to bump the feature-test macro from

#define __cpp_lib_ranges 202106L


#define __cpp_lib_ranges 202110L

but we can't do that because we don't implement all of 202106 Ranges yet.

This is intended to supersede @CaseyCarter's D116885, by just making us do the right thing with those tests instead.

Diff Detail

Event Timeline

Quuxplusone created this revision.Jan 9 2022, 9:38 AM
Quuxplusone requested review of this revision.Jan 9 2022, 9:38 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptJan 9 2022, 9:38 AM
ldionne requested changes to this revision.Jan 11 2022, 8:41 AM

A couple of comments, but overall looks pretty good. Thanks!


You're not using this header anymore, I think.


Please make sure your indentation is consistent (either 2 or 4 spaces, but don't mix the two in the same file).


I found it weird to reuse the same type for testing const and non-const, and using the fact that the const versions return char* and the non-const versions int*. I was going to ask to use two separate view types instead, but reconsidering it, I think it's easier to follow as-is for some reason that escapes me. No action here, just thinking out loud.


Can you one-line comment these test cases?


You are missing initializer_list, and also the tests for it below AFAICT.


In the paper, they mention this:

Editor's note: remove_reference_t rather than remove_cvref_t because we need to reject const vector<int>&& from being a viewable_range

I don't think we have a test that a Range const&& isn't a viewable_range -- could we add one? In other words, I'd like a test that fails if you had used movable<remove_cvref_t<_Tp>> instead of movable<remove_reference_t<_Tp>>.

This revision now requires changes to proceed.Jan 11 2022, 8:41 AM
Quuxplusone marked 5 inline comments as done.Jan 12 2022, 6:54 AM
Quuxplusone added inline comments.

My understanding is that this test relies on the ["clever" use of T to mean T&&]( that I've been railing against all this week for some reason. ;)
So when you see static_assert(!std::ranges::viewable_range<const T6>); above, that's basically the const vector<int>&& case — it's testing that a (range, non-view, const, rvalue) is not a viewable_range.

However, if you ask me to rewrite all the above <X>s to <X&&>s, or to add <X&&>s in addition to the <X>s, I'll be reasonably happy to do so!

Quuxplusone marked an inline comment as done.

Add a test proving initializer_list<int> is not a viewable_range.
Address review comments.

ldionne added inline comments.Jan 12 2022, 8:21 AM

I think we should add X&& in addition to X. That would make it clear that we cover the case I mentioned above (and which is called out explicitly in the paper).

Add <X&&> tests to viewable_range.compile.pass.cpp.
Remove one trailing space.

ldionne accepted this revision.Jan 12 2022, 1:27 PM
This revision is now accepted and ready to land.Jan 12 2022, 1:27 PM

Fix a GCC shadowing warning in the test.
Looks like Clang 12 incorrectly treated same_as<T> decltype(auto) as a synonym for same_as<T> auto.

Just stop using constrained decltype(auto).