-
-
Notifications
You must be signed in to change notification settings - Fork 14.1k
Fix std::mem::drop rustdoc misleading statement #149244
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
|
r? @ibraheemdev rustbot has assigned @ibraheemdev. Use |
This comment has been minimized.
This comment has been minimized.
b0c39a1 to
13fc0d8
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
| } | ||
| } | ||
|
|
||
| /// Disposes of a value. |
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.
| /// Disposes of a value. | |
| /// Disposes of a value by taking ownership of it and letting it go out of scope (which invokes its [`Drop`][drop] implementation). |
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 kind of goes against the original purpose of this change.
Sure, Drop is invoked when something goes out of scope, but the clarification is that this function does not actually interact with Drop at all directly; it's just an empty function body.
This change puts the clarification that it's just an empty function body before the actual mention of [Drop], making things more clear.
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.
The "letting it go out of scope" is just a bit more general and direct than saying "just an empty function body", otherwise you still need to infer that an empty function makes all its arguments go out of scope...
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.
Agree with @clarfonthey, I think there are a lot of ways to explain this and this is why I've decided to just do the minimal possible thing until somebody maybe from @rust-lang/libs-contributors comes and suggest a way in which they would approve the change.
I guess the best possible way is to say ... very informal version ... This is just an empty function that works like any other empty function which takes ownership of the value, at the end, due to what the compiler does in the process of mir building which injects Drop Terminators, drop elaboration refines this and then we would insert the corresponding drop glue, then potentially calling Drop::drop. Something / something like that, or maybe that is too technical for libs documentation and we don't want to explain things in terms of what happens inside the compiler.
Maybe just, This is just an empty function that works like any other empty function which takes ownership of the value, at the end, when the value goes out of scope it gets droppped.
13fc0d8 to
d60f780
Compare
|
Thanks! @bors r+ rollup |
|
🌲 The tree is currently closed for pull requests below priority 1000. This pull request will be tested once the tree is reopened. |
Rollup of 5 pull requests Successful merges: - #148918 (Remove an outdated test) - #149244 (Fix std::mem::drop rustdoc misleading statement) - #149532 (Rename supertrait item shadowing lints) - #149541 (Various never type test improvements) - #149590 (linker: Remove special case for `rust-lld` in `detect_self_contained_mingw`) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #149244 - spastorino:fix-mem-drop-rustdoc, r=ChrisDenton Fix std::mem::drop rustdoc misleading statement This is a bit misleading, we were discussing this with our Rust team and some people could think that the compiler does some special magic for this specific function and that's not true or well the compiler does something special but for every function. The reality according to my understanding is that this is a normal function that takes ownership of the given value and as with every other function mir building injects Drop Terminators , drop elaboration refines this and then we would insert the corresponding drop glue, then potentially calling Drop::drop. Not sure if it would be best to remove the sentence as this PR does or explaining something along the lines of the previous text.
This is a bit misleading, we were discussing this with our Rust team and some people could think that the compiler does some special magic for this specific function and that's not true or well the compiler does something special but for every function.
The reality according to my understanding is that this is a normal function that takes ownership of the given value and as with every other function mir building injects Drop Terminators , drop elaboration refines this and then we would insert the corresponding drop glue, then potentially calling Drop::drop.
Not sure if it would be best to remove the sentence as this PR does or explaining something along the lines of the previous text.