Address concerns of weak-into-raw #71107
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Rename Weak::as_raw to Weak::as_ptr for consistency with some other types. * The as_ptr for a dangling Weak pointer might return whatever garbage (and takes that advantage to avoid a conditional). * Don't guarantee to be able to do `Weak::from_raw(weak.as_ptr())` (even though it'll still work fine).
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
r? @Amanieu |
|
cc @RalfJung |
RalfJung
reviewed
Apr 14, 2020
Amanieu
reviewed
Apr 18, 2020
|
r=me once the issues with the doc comments are fixed. |
For consistency with Weak
vorner
force-pushed
the
weak-into-raw-dangling
branch
from
eb84a49 to
f4ded11
Compare
|
Thanks. I've forced-pushed it directly, but the only change is the rewording of the comment. |
|
@bors r+ rollup |
|
📌 Commit f4ded11 has been approved by |
bors
added
S-waiting-on-bors
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
Apr 19, 2020
…anieu Address concerns of weak-into-raw This should address the standing concerns in rust-lang#60728 (comment). I've still left the ability to create a new dangling pointer from `null`, as I feel like this is the natural behaviour to expect, but I'm fine removing that too. I've modified the documentation to allow the `as_ptr` or `into_ptr` to return whatever garbage in case of a dangling pointer. I've also removed the guarantee to be able to do `from_raw(as_ptr)` from the documentation (but it would still work right now). I've renamed the method and added implementations for `Rc`/`Arc`. I've also tried if I can just „enable“ unsized types. I believe the current interface is compatible with them. But the inner implementation will be a bit challenging ‒ I can't use the `data_offset` as is used by `Rc` or `Arc` because it AFAIK „touches“ (creates a reference to) the live value of `T` ‒ and in case of `Weak`, it might be completely bogus or already dead ‒ so that would be UB. `./x.py test tidy` is completely mad on my own system all over the code base :-(. I'll just hope it goes through CI, or will fix as necessary. Is it OK if I ask @Amanieu for review, as the concerns are from you? ~r @Amanieu
This was referenced Apr 19, 2020
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Apr 19, 2020
Rollup of 5 pull requests Successful merges: - rust-lang#71107 (Address concerns of weak-into-raw) - rust-lang#71188 (Fixed missing trait method suggests incorrect code (self parameter not named "self"). ) - rust-lang#71300 (Clarify when to use the tracking issue template) - rust-lang#71315 (Add example in the alternative in std::mem::transmute docs) - rust-lang#71319 (Clean up E0522 explanation) Failed merges: r? @ghost
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This should address the standing concerns in #60728 (comment).
I've still left the ability to create a new dangling pointer from
null, as I feel like this is the natural behaviour to expect, but I'm fine removing that too. I've modified the documentation to allow theas_ptrorinto_ptrto return whatever garbage in case of a dangling pointer. I've also removed the guarantee to be able to dofrom_raw(as_ptr)from the documentation (but it would still work right now).I've renamed the method and added implementations for
Rc/Arc.I've also tried if I can just „enable“ unsized types. I believe the current interface is compatible with them. But the inner implementation will be a bit challenging ‒ I can't use the
data_offsetas is used byRcorArcbecause it AFAIK „touches“ (creates a reference to) the live value ofT‒ and in case ofWeak, it might be completely bogus or already dead ‒ so that would be UB../x.py test tidyis completely mad on my own system all over the code base :-(. I'll just hope it goes through CI, or will fix as necessary.Is it OK if I ask @Amanieu for review, as the concerns are from you?
~r @Amanieu