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.

Sign up for GitHub

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

Jump to bottom

Implement ranges::common_view #1305

Merged
merged 18 commits into from
Nov 4, 2020
Merged

Implement ranges::common_view #1305

merged 18 commits into from
Nov 4, 2020

Conversation

miscco
Copy link
Contributor

@miscco miscco commented Sep 17, 2020

Partially addresses #39

Currently a WIP as testing views is just wonderfully hard

@CaseyCarter CaseyCarter added cxx20 C++20 feature ranges C++20/23 ranges labels Sep 17, 2020
CaseyCarter
CaseyCarter suggested changes Sep 17, 2020
View reviewed changes
Copy link
Contributor

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

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

Looks like a good start. (I find it terribly ironic that you added no tests yet the tests are not passing ;))

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 Outdated Show resolved Hide resolved
@miscco
Copy link
Contributor Author

miscco commented Sep 18, 2020

Looks like a good start. (I find it terribly ironic that you added no tests yet the tests are not passing ;))

That is my secret superpower. I can break code just by looking at it.

Reminds me when I had to rework the C-code generator of the compiler I was working on. Bugs that cancel each other out, depend on the language settings of the machine....

@AdamBucior
Copy link
Contributor

While merging branches you've cloned your changes and everything is defined twice.

CaseyCarter
CaseyCarter suggested changes Sep 20, 2020
View reviewed changes
stl/inc/ranges Outdated Show resolved Hide resolved
stl/inc/ranges Outdated Show resolved Hide resolved
stl/inc/ranges Outdated Show resolved Hide resolved
@miscco
Copy link
Contributor Author

miscco commented Sep 20, 2020 •
edited
Loading

I will simply go back and rebase unto master after i fixed your comment

@miscco
Copy link
Contributor Author

miscco commented Sep 21, 2020

Rebased and removed the duplicated code

AdamBucior
AdamBucior reviewed Sep 21, 2020
View reviewed changes
stl/inc/ranges Outdated Show resolved Hide resolved
@miscco @AdamBucior
Update stl/inc/ranges
4ccce08 
Co-authored-by: Adam Bucior <[email protected]>
AdamBucior
AdamBucior reviewed Sep 21, 2020
View reviewed changes
stl/inc/ranges Outdated Show resolved Hide resolved
@miscco @AdamBucior
Update stl/inc/ranges
4f9eb68 
Co-authored-by: Adam Bucior <[email protected]>
CaseyCarter
CaseyCarter reviewed Sep 22, 2020
View reviewed changes
Copy link
Contributor

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

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

What's here looks fine - just missing test coverage. (On the bright side, common_view doesn't have custom iterator or sentinel types to test.)

@miscco
Copy link
Contributor Author

miscco commented Sep 22, 2020

Yeah, I started, but my kids decided that sleep is totally not worth it so I am moving at a crawl right now

@miscco
Copy link
Contributor Author

miscco commented Sep 26, 2020

This is hell, I need to seriously consider to what extend we can use constexpr here ...

miscco
miscco commented Sep 26, 2020
View reviewed changes
tests/std/tests/P0896R4_views_common/test.cpp Outdated
}

// Validate view_interface::data
static_assert(!CanData<R>);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I find this super troubling. If the user passes a continuous_range into common_view he will be in for a surprise

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a backwards-compatibility tool to adapt non-common_ranges into an iterator pair that can be passed to C++17 algorithms. It doesn't need to have great support for contiguous_ranges that don't exist in C++17.

@miscco miscco marked this pull request as ready for review September 28, 2020 13:10
@miscco miscco requested a review from a team as a code owner September 28, 2020 13:10
@CaseyCarter CaseyCarter mentioned this pull request Sep 29, 2020
Closed
@CaseyCarter CaseyCarter self-assigned this Sep 30, 2020
StephanTLavavej
StephanTLavavej requested changes Oct 23, 2020
View reviewed changes
stl/inc/ranges Outdated Show resolved Hide resolved
stl/inc/ranges Outdated Show resolved Hide resolved
tests/std/tests/P0896R4_views_common/test.cpp Show resolved Hide resolved
tests/std/tests/P0896R4_views_common/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P0896R4_views_common/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P0896R4_views_common/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P0896R4_views_common/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P0896R4_views_common/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P0896R4_views_common/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P0896R4_views_common/test.cpp Outdated Show resolved Hide resolved
@miscco miscco force-pushed the common_view branch 3 times, most recently from d0a3ed7 to 865fa91 Compare October 23, 2020 20:54
@miscco
Copy link
Contributor Author

miscco commented Oct 31, 2020 •
edited
Loading

Could I get an "thumbs up" / "looks ok" / "burn it with fire" regarding the _Choice_t strategy for exception specifications?

I am currently inclined to simply revert it, So I did not yet spend time debugging build failure

@CaseyCarter CaseyCarter mentioned this pull request Nov 2, 2020
Merged
StephanTLavavej
StephanTLavavej reviewed Nov 3, 2020
View reviewed changes
Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

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

Looks good! I will fix one typo.

tests/std/tests/P0896R4_views_common/test.cpp Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej removed their assignment Nov 3, 2020
@StephanTLavavej

This comment has been minimized.

Sign in to view
@azure-pipelines

This comment has been minimized.

Sign in to view
@CaseyCarter CaseyCarter self-assigned this Nov 3, 2020
@CaseyCarter CaseyCarter merged commit 332fd32 into microsoft:master Nov 4, 2020
@CaseyCarter
Copy link
Contributor

Thanks for your contribution!

@miscco miscco deleted the common_view branch November 7, 2020 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AdamBucior AdamBucior AdamBucior left review comments

@CaseyCarter CaseyCarter CaseyCarter approved these changes

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

Assignees

@CaseyCarter CaseyCarter

Labels
cxx20 C++20 feature ranges C++20/23 ranges
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@miscco @AdamBucior @StephanTLavavej @CaseyCarter