-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Add note about Copy for drop() #28319
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
Conversation
I've always been a bit wary of introducing things like this into examples, because what people get confused by is always different, and deciding what "obvious" things is hard on each example. But I'm okay with this. |
@bors: r+ rollup |
📌 Commit 1305ab3 has been approved by |
@@ -434,6 +434,10 @@ pub fn replace<T>(dest: &mut T, mut src: T) -> T { | |||
/// While this does call the argument's implementation of `Drop`, it will not | |||
/// release any borrows, as borrows are based on lexical scope. | |||
/// | |||
/// This does nothing for types which implement `Copy`, i.e. integers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"e.g. integers"
@bors r=steveklabnik rollup |
📌 Commit 1305ab3 has been approved by |
1305ab3
to
fd40372
Compare
@steveklabnik
[EDIT] Added new to the previous line. |
I think as far as documentation goes copy semantics have always been portrayed as being different from move semantics (not "a move and then a copy") |
No, a copy and then a move. The original is untouched but ownership transference still applies to the new copy. Is there a reason to attempt to separate these two topics otherwise?They're on the same page and move leads directly into copy. It just cements further the idea that ownership move semantics are a pervasive idea. |
Yeah, I would say i agree with @mdinger in the sense that we do try to make it explicit that the only difference is the ability to use it after, or at least, if we're not, we should. |
My preference would definitely be the footnote. Then you give them a guiding push in the correct direction and the real location of where this is documented. If they still don't understand, then maybe the regular docs need further improvement too. |
13cf97b
to
3299e80
Compare
@steveklabnik updated |
/// This effectively does nothing for | ||
/// [types which implement `Copy`](http://doc.rust-lang.org/nightly/book/ownership.html#copy-types), | ||
/// e.g. integers. Such values are copied and _then_ moved into the function, | ||
/// so the new copy persists after this function call. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This disagrees slightly with the examples below where they state the copy is moved and dropped. Here it states the the new copy persists which is kinda the opposite. They should at least agree.
I don't know which way it's implemented or if it matters but from a explanation standpoint, letting someone copy your item so they have a new copy is more typical. It'd be more unusual if you let them copy and kept the new copy leaving them the ratty old copy you have (imagine copying homework or recipes for example).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
3299e80
to
f2707ab
Compare
@@ -434,6 +434,11 @@ pub fn replace<T>(dest: &mut T, mut src: T) -> T { | |||
/// While this does call the argument's implementation of `Drop`, it will not | |||
/// release any borrows, as borrows are based on lexical scope. | |||
/// | |||
/// This effectively does nothing for | |||
/// [types which implement `Copy`](http://doc.rust-lang.org/nightly/book/ownership.html#copy-types), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, one more nit: this needs to be a relative, not absolute, link
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
f2707ab
to
012f369
Compare
@bors: r+ rollup |
📌 Commit 012f369 has been approved by |
https://users.rust-lang.org/t/confused-by-std-mem-drop/2783/3
r? @gankro