Improve mem::forget documentation #25861
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
| /// There's only a few reasons to use this function. The mainly come | ||
| /// up in unsafe code or FFI code. | ||
| /// | ||
| /// * You have an uninitialed value, perhaps for performance reasons, and |
|
Thanks so much for this patch! Just a few nits. |
| /// let mut t: T = uninitialized(); | ||
| /// | ||
| /// // Perform the swap, `&mut` pointers never alias | ||
| /// ptr::copy_nonoverlapping(&*x, &mut t, 1); |
There was a problem hiding this comment.
I think that this code example will need to import std::ptr to compile, as well as change the call to uninitialized to mem::uninitialized. You can be sure to run the doc tests via make check-stage1-doc-crate-core.
There was a problem hiding this comment.
also, it would be nice to split up the different examples into different code blocks. right now, the whole thing is no_run because of the file example, but the others can be run.
|
@alexcrichton @steveklabnik Ok, I broke it up into 3 code blocks. The last one is still failing when I run |
|
@tringenbach what's the output of that |
|
@tringenbach if you push that commit up to github, I can check it out too :) |
|
@steveklabnik Fair enough. I was trying to avoid pushing something I knew was broken. It's pushed now. @alexcrichton It just says that forget_2 failed. [...] failures: test result: FAILED. 436 passed; 1 failed; 5 ignored; 0 measured |
|
You forgot a space before your closing backticks. I have one more nit, so I sent you tringenbach#1 😄 Merge that in, squash this into one commit with |
|
@steveklabnik Even after merging your pull request, I still get a failure: test mem::forget_2 ... FAILED That is on running |
|
Ah! I wonder how I didn't see this before. It's an easy fix. The issue is So, just remove the |
This adds an example from mem::swap, and provides some suggested uses of this function. Change wording on comment on forget line to be more specific as to why we need to call forget. This breaks the examples up into three pieces. The last piece isn't compiling for some reason.
|
@steveklabnik Probably because I didn't originally have the "pub", but added it in a later commit to try to make it work. How did you get that more detailed out? Anyway, I made the fix, no more failures, so I squashed it into one commit and did a 'git push -f'. |
|
@bors: r+ rollup |
|
📌 Commit 8746b1a has been approved by |
This adds an example from mem::swap, and provides some suggested uses of this function. This is my attempt to summarize the answers to a question I asked on reddit http://www.reddit.com/r/rust/comments/37jcul/what_is_forget_for/ and add the answers to the documentation so that no one else has to google or ask the question again.
This adds an example from mem::swap, and provides some suggested uses of this function. This is my attempt to summarize the answers to a question I asked on reddit http://www.reddit.com/r/rust/comments/37jcul/what_is_forget_for/ and add the answers to the documentation so that no one else has to google or ask the question again.
This adds an example from mem::swap, and provides some suggested uses of this
function.
This is my attempt to summarize the answers to a question I asked on reddit http://www.reddit.com/r/rust/comments/37jcul/what_is_forget_for/ and add the answers to the documentation so that no one else has to google or ask the question again.