Document return value of zero-size/zero-heap pointer types. #39757
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (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. Due to 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. |
|
Not too satisfied with the term 'meaningless' in my changes, so please let me know if you know a better way of wording this. |
frewsxcv
added
the
A-docs
| @@ -282,6 +282,9 @@ impl<T> Arc<T> { | |||
| /// To avoid a memory leak the pointer must be converted back to an `Arc` using | |||
| /// [`Arc::from_raw`][from_raw]. | |||
| /// | |||
| /// If `T` is zero-sized (e.g. `Arc<()>`), the returned pointer address | |||
| /// will be meaningless. | |||
There was a problem hiding this comment.
Nit for myself: I can probably just s/will be meaningless/is meaningless/.
There was a problem hiding this comment.
I think for Rc and Arc this is wrong, because from_raw exists and there's the two refcounts, not just the ZST.
There was a problem hiding this comment.
@eddyb can you elaborate on what you mean here? This method returns a pointer to the contents, right?
But it does seem weird to simultaneously say that the return value is meaningless, and then have from_raw say that the result must have been generated by into_raw.
I guess I'm not sure what meaningless means, really.
There was a problem hiding this comment.
The value is one-byte-past-the-refcounts and can be used to get back an Rc, and you can distinguish between them, so definitely not meaningless (whereas an &ZST by itself is useless for anything short of a proof that there is a value of type ZST somewhere).
|
I wonder if we could explore why we need to clarify this? All of these returned pointers are valid for 0 bytes, so it seems like that's what we should document rather than explicitly calling out the "empty cases" for all situations here (if at all). It also seems to me that if a pointer is valid for 0 bytes it doesn't really matter what it ends up being in practice because you still can't read it. |
|
It is not meaningless, either. You must use the same pointer returned by these functions to reconstruct the smart pointers in order to have the correct behaviour when these smart pointers are destructed. |
|
I think the important thing to emphasize is that the pointer is always non-null. |
src/libstd/ffi/c_str.rs
Outdated
| @@ -245,6 +245,9 @@ impl CString { | |||
|
|
|||
| /// Transfers ownership of the string to a C caller. | |||
| /// | |||
| /// If the `CString` is empty, the returned pointer address will be | |||
| /// meaningless. | |||
There was a problem hiding this comment.
For CString and CStr the pointer is definitely not meaningless as even if the string is empty it will still point to a single zero byte.
There was a problem hiding this comment.
Good call. Reverted in the latest force push.
Did you see the original issue? #39625
I read this a few times and still don't really understand what this means. Which smart pointers are you referring to here? I don't feel strongly about these changes, so if people here think these changes are unnecessary, please go give feedback in the original issue and then we could possibly close that and this PR. |
frewsxcv
force-pushed
the
zero-pointer
branch
from
ded08e0
to
525f3df
Compare
|
cc @rust-lang/compiler -- can someone take on the review here? |
I’ll gladly look over, but this is one of those issues that also may span to T-lang and T-libs depending on how documentation gets worded. |
|
This is tough, but I think @nagisa has good points. |
|
☔ The latest upstream changes (presumably #39876) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Thanks everyone for the feedback. Considering the feedback I got, it seems like there's not much we can document here. Unless anyone knows actionable items I can take regarding this, I'll plan on closing this. |
Fixes #39625.