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

clone_from behaviour for ManuallyDrop #86288

Closed
m-ou-se opened this issue Jun 14, 2021 · 3 comments · Fixed by #86880
Closed

clone_from behaviour for ManuallyDrop #86288

m-ou-se opened this issue Jun 14, 2021 · 3 comments · Fixed by #86880
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@m-ou-se
Copy link
Member

m-ou-se commented Jun 14, 2021

With #[derive(Clone)] on ManuallyDrop<T>, x.clone_from(&y) will not drop the T inside x, just like x = y.clone(); does not drop the original x before overwriting it.

However, a manual implementation of clone_from can only make use of the efficiency of a overriden clone_from on T (e.g. memory reuse by a Vec::clone_from) by simply forwarding to it, which means that x.clone_from(&y) will not forget the old value.

This new (forwarding, so (potentially) dropping) behaviour was added in #85176, and (for now) reverted in #85758.

So question for @rust-lang/libs: What behaviour would be correct? There are arguments for both. Whatever the decision, we should document it and add a test for it.

@m-ou-se m-ou-se added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jun 14, 2021
@Amanieu
Copy link
Member

Amanieu commented Jun 15, 2021

I feel that the old behavior is correct (not calling drop). clone_from should behave like x = y.clone() which for ManuallyDrop means not dropping the old value.

I don't think there is much loss of efficiency in practice: use of clone_from is already pretty rare and I don't think there are any cases of it being used with ManuallyDrop.

@m-ou-se
Copy link
Member Author

m-ou-se commented Jul 5, 2021

Sounds good. Closing this.

@m-ou-se m-ou-se closed this as completed Jul 5, 2021
@m-ou-se
Copy link
Member Author

m-ou-se commented Jul 5, 2021

Whatever the decision, we should document it and add a test for it.

No wait, we still need to do this part. :)

@m-ou-se m-ou-se reopened this Jul 5, 2021
@m-ou-se m-ou-se linked a pull request Jul 5, 2021 that will close this issue
JohnTitor added a commit to JohnTitor/rust that referenced this issue Jul 7, 2021
…m, r=Mark-Simulacrum

Test ManuallyDrop::clone_from.

See rust-lang#86288
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants