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::uninitialized_meow #1164

Merged

Conversation

miscco
Copy link
Contributor

@miscco miscco commented Aug 8, 2020

This ports the uninitialized_default_construct{_n} algorithms to ranges.

NOTE: This is based upon the uninitialized_move PR as that PR is quite advanced and the merge conflicts by replicating bits of it didnt seem worth it. So it makes sense to only look at the final commit

@miscco miscco requested a review from a team as a code owner August 8, 2020 12:18
@miscco
Copy link
Contributor Author

miscco commented Aug 8, 2020

I hade some time at hand and added uninitialized_value_construct and uninitialized_fill

@miscco miscco changed the title Ranges uninitialized default construct Ranges uninitialized *meow* Aug 8, 2020
@StephanTLavavej StephanTLavavej added cxx20 C++20 feature ranges C++20/23 ranges labels Aug 8, 2020
@miscco miscco changed the title Ranges uninitialized *meow* Implement ranges::uninitialized_meow Aug 9, 2020
@CaseyCarter CaseyCarter mentioned this pull request Aug 9, 2020
@miscco miscco force-pushed the ranges_uninitialized_default_construct branch 3 times, most recently from e1decd0 to 6136e62 Compare August 10, 2020 19:59
@CaseyCarter CaseyCarter self-assigned this Aug 12, 2020
@CaseyCarter CaseyCarter force-pushed the ranges_uninitialized_default_construct branch from de904c7 to 7ee1c3e Compare August 26, 2020 21:09
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.

I haven't dug into the tests yet, but there is plenty of product code feedback here. ;)

* `ranges::uninitialized_default_construct`, `ranges::uninitialized_default_construct_n`
* `ranges::uninitialized_value_construct`, `ranges::uninitialized_value_construct_n`
* `ranges::uninitialized_fill`, `ranges::uninitialized_fill_n`
* `ranges::uninitialized_copy`, `ranges::uninitialized_copy_n`
* `ranges::uninitialized_move_n`
Copy link
Contributor Author

@miscco miscco left a comment

Choose a reason for hiding this comment

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

I started, will continue tomorrow

@miscco miscco force-pushed the ranges_uninitialized_default_construct branch from 7ee1c3e to c925844 Compare September 2, 2020 19:18
@miscco
Copy link
Contributor Author

miscco commented Sep 2, 2020

@CaseyCarter Thanks a lot for the comments.

I incorporated them and will have a look at additional test coverage soonTM

@StephanTLavavej
Copy link
Member

This needs to be merged with recent changes to <memory>.

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.

This is getting close.

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.

This should be ready to merge sometime in 2023 when we can convince someone else to review it ;)

@CaseyCarter CaseyCarter removed their assignment Sep 17, 2020
@StephanTLavavej StephanTLavavej self-assigned this Sep 17, 2020
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.

I believe I found some minor issues but @CaseyCarter should verify as my brain is running low on caffeine.

@StephanTLavavej StephanTLavavej removed their assignment Sep 18, 2020
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 to me now except for one unresolved comment. @CaseyCarter feel free to move this to Ready To Merge.

@CaseyCarter CaseyCarter removed their assignment Sep 18, 2020
@StephanTLavavej
Copy link
Member

@miscco @CaseyCarter I applied @AdamBucior's suggested change after verifying that no other occurrences were affected (either in this PR or in the codebase). We have a new convention to remember! 😺

@miscco
Copy link
Contributor Author

miscco commented Sep 19, 2020

Thanks a lot, I thought I found all of those

@StephanTLavavej StephanTLavavej merged commit 5f4bc22 into microsoft:master Sep 22, 2020
@StephanTLavavej
Copy link
Member

Thanks for constructing so many cat noises in uninitialized memory! 😺 😸 🐱 🐈

@miscco miscco deleted the ranges_uninitialized_default_construct branch September 30, 2020 14:30
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