This is an archive of the discontinued LLVM Phabricator instance.

[libc++] [ranges] Implement P2415R2 owning_view
ClosedPublic

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

Details

Summary

"What is a view?"
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p2415r2.html
https://github.com/cplusplus/draft/pull/5010/files

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

to

#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!

libcxx/test/std/ranges/range.adaptors/range.all/all_t.compile.pass.cpp
18–19

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

libcxx/test/std/ranges/range.adaptors/range.all/range.owning.view/base.pass.cpp
26–29

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

libcxx/test/std/ranges/range.adaptors/range.all/range.owning.view/begin_end.pass.cpp
29

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.

91

Can you one-line comment these test cases?

libcxx/test/std/ranges/range.req/range.refinements/viewable_range.compile.pass.cpp
26–27

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

161

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.
libcxx/test/std/ranges/range.req/range.refinements/viewable_range.compile.pass.cpp
161

My understanding is that this test relies on the ["clever" use of T to mean T&&](https://reviews.llvm.org/D116991#inline-1119810) 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
libcxx/test/std/ranges/range.req/range.refinements/viewable_range.compile.pass.cpp
161

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.

https://buildkite.com/llvm-project/libcxx-ci/builds/7791
UNSUPPORTED: clang-12
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).

libcxx/test/std/ranges/range.adaptors/range.reverse/ctad.compile.pass.cpp