Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement ranges::ref_view #1132

Merged
merged 7 commits into from
Aug 22, 2020
Merged

Conversation

miscco
Copy link
Contributor

@miscco miscco commented Aug 2, 2020

This implements ranges::ref_view which is a precursor for most other range adaptors.

NOTE: I need to come up with a reasonably simple test for the converting onstructor from a different range.

@CaseyCarter: Should we add empty to test::range?

@miscco miscco requested a review from a team as a code owner August 2, 2020 17:18
@StephanTLavavej StephanTLavavej added cxx20 C++20 feature ranges C++20/23 ranges labels Aug 3, 2020
stl/inc/ranges Outdated Show resolved Hide resolved
stl/inc/ranges Outdated Show resolved Hide resolved
stl/inc/ranges Outdated Show resolved Hide resolved
stl/inc/ranges Outdated Show resolved Hide resolved
stl/inc/ranges Outdated Show resolved Hide resolved
stl/inc/ranges Show resolved Hide resolved
tests/std/tests/P0896R4_ranges_ref_view/test.cpp Outdated Show resolved Hide resolved
@miscco
Copy link
Contributor Author

miscco commented Aug 5, 2020

Now i remember, it was actually not a squirrel but some three headed beast that let the poison pill fail to compile

@miscco
Copy link
Contributor Author

miscco commented Aug 5, 2020

Found the bug, it was sitting in front of the monitor...

@AdamBucior
Copy link
Contributor

ref_view should not be in std::ranges::views

@miscco
Copy link
Contributor Author

miscco commented Aug 6, 2020

@AdamBucior thanks, I was confused by the range adaptor sentence that everything lives in std::ranges::views

@CaseyCarter
Copy link
Member

@CaseyCarter: Should we add empty to test::range?

Why?

@miscco
Copy link
Contributor Author

miscco commented Aug 6, 2020

@CaseyCarter: Should we add empty to test::range?

Why?

because then I would not need to go with span to test it ;)

stl/inc/ranges Outdated Show resolved Hide resolved
stl/inc/ranges Outdated Show resolved Hide resolved
stl/inc/ranges Outdated Show resolved Hide resolved
stl/inc/ranges Outdated Show resolved Hide resolved
stl/inc/ranges Outdated Show resolved Hide resolved
tests/std/tests/P0896R4_ranges_ref_view/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P0896R4_ranges_ref_view/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P0896R4_ranges_ref_view/test.cpp Outdated Show resolved Hide resolved
@CaseyCarter
Copy link
Member

because then I would not need to go with span to test it ;)

I figured this out during my detailed review; see my comments inline which boil down to "write a concept to determine if ranges::empty(Read{}) is valid and ensure that ref_view<Read>::empty is valid iff that concept is satisfied."

stl/inc/ranges Outdated Show resolved Hide resolved
stl/inc/ranges Outdated Show resolved Hide resolved
stl/inc/ranges Outdated Show resolved Hide resolved
tests/std/tests/P0896R4_ranges_ref_view/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P0896R4_ranges_ref_view/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P0896R4_ranges_ref_view/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P0896R4_ranges_ref_view/test.cpp Outdated Show resolved Hide resolved
@miscco
Copy link
Contributor Author

miscco commented Aug 21, 2020

FYI I am out until Monday, so if this is blocking you feel free to adopt whatever you need.

I will pick up the remnants later

@CaseyCarter CaseyCarter removed their assignment Aug 21, 2020
constexpr ref_view(_OtherRng&& _Other) noexcept(
noexcept(static_cast<_Rng&>(_STD forward<_OtherRng>(_Other)))) // strengthened
requires convertible_to<_OtherRng, _Rng&> && requires {
_Rvalue_poison(static_cast<_OtherRng&&>(_Other));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I observe that the requires says static_cast<_OtherRng&&>(_Other) instead of _STD forward<_OtherRng>(_Other). I am not sure if the variation is intentional. No change requested.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is intentional: I try to avoid instantiating and/or specializing templates during overload resolution for throughput. In this case, that means static_cast<T&&>(t) in the constraints instead of std::forward<T>(t) that is used in noexcept-specifiers and function template bodies which are instantiated after overload resolution. There's an argument to be made that the difference is anomalous enough to offset any readability benefit.

@StephanTLavavej StephanTLavavej self-assigned this Aug 21, 2020
@StephanTLavavej StephanTLavavej merged commit aafee76 into microsoft:master Aug 22, 2020
@StephanTLavavej
Copy link
Member

Thanks for implementing these ranges features so quickly! It might be a world record, we should call a ref over to view the instant replay. 🥇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cxx20 C++20 feature ranges C++20/23 ranges
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants