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

Add <[T; N]>::copied #76236

Closed
wants to merge 2 commits into from
Closed

Conversation

scottmcm
Copy link
Member

@scottmcm scottmcm commented Sep 2, 2020

This works (it's a nicer version of how I did things in ArrayTools), so I figured I'd just send a PR and see what people think about it.

r? @LukasKalbertodt
cc #76118

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 2, 2020
@LukasKalbertodt
Copy link
Member

I really want that method (and cloned), but I would prefer not using this kind of workaround :/
I think the correct solution would be like Option<T> does it: impl<T: Copy> Option<&T> { ... }. For that we need a way to add multiple impl blocks, which is not currently possible with only one lang item. I started a discussion on Zulip about this. Hopefully T-lang can help out :)

Comment on lines +457 to +459
for i in 0..N {
dst[i].write(self[i].copy());
}
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, do we get "guaranteed" bounds check elimination here? That is, am I correct that rewriting this as dst.iter_mut().zip(self.iter()) would make this strictly worse?

Copy link
Member Author

@scottmcm scottmcm Sep 2, 2020

Choose a reason for hiding this comment

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

After mir-opt the check is against a constant, which is quite easy for LLVM to remove. (The for i in 0..N loop is easily canonicalized, especially now that Range<usize>::next uses add nuw.)

Copy link
Contributor

@pickfire pickfire Sep 4, 2020

Choose a reason for hiding this comment

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

Must we write byte by byte if it implements copy? Oh, I forgot it is a reference not original value.

@pickfire
Copy link
Contributor

pickfire commented Sep 4, 2020

I believe the next thing we will be adding is cloned?

@scottmcm
Copy link
Member Author

Given #76236 (comment) I'll just close this.

@scottmcm scottmcm closed this Sep 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants