Add Weak.ptr_eq #55987
Add Weak.ptr_eq #55987
Conversation
|
r? @sfackler (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
There's no ABA problem here since the underlying Rc allocation isn't freed while there are outstanding Weaks, right? I agree with the behavior for dangling Weaks. We should define this for both Arc's and Rc's Weak. |
|
@sfackler the memory is only deallocated once the weak count hits 0 (starts at 1), see https://github.com/rust-lang/rust/blob/master/src/liballoc/rc.rs#L855-L857 and https://github.com/rust-lang/rust/blob/master/src/liballoc/rc.rs#L1297-L1301. As for the dangling pointers; I don't think |
|
Ok cool, this approach should work then. Let's add the same method for Arc and this should be good to go. |
|
cc @rust-lang/libs |
|
Do you want the Also what should I do with the stable/unstable attribute? |
|
Let's add it in this PR. The new functions should be added as unstable initially. We can repurpose #55981 as the tracking issue. |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
oh, we should also add some basic tests. |
|
@sfackler I've add examples which also serve as tests. I explicitly added the example of the dangling pointer. What other tests would you like to see? |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
☔ The latest upstream changes (presumably #55627) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Anything I can do to progress this? |
|
Oh whoops - Github doesn't email on force pushes so I didn't see it was rebased :( @bors r+ |
|
📌 Commit 90d2914da3d15ebe8ea0c95de2b3270ef3c8bddb has been approved by |
bors
added
S-waiting-on-bors
src/liballoc/sync.rs
Outdated
| /// | ||
| /// When using `Weak::new` the `Weak` is never equal to any other `Weak` | ||
| /// until it points to actual data (e.g. by setting it to a value created by | ||
| /// calling [`Arc::downgrade`]). |
There was a problem hiding this comment.
I think this documentation is slightly confusion. Outside of std, I don't think there is any way to turn a reference obtained through Weak::new into a reference to something real. I think it should just say something like:
References obtained through
Weak::neware not equal to any other references, including other invocations ofWeak::new.
(same applies to rc::Weak)
There was a problem hiding this comment.
Outside of std, I don't think there is any way to turn a reference obtained through Weak::new into a reference to something real.
I'm not sure what you mean by that. You can overwrite the value, e.g.
let mut weak_value = Weak::new();
let value = Rc::new(5);
weak_value = Rc::dowgrade(&value);As for the comparing two Weak::new() with another I agree, but isn't the example enough? Currently no pointer created via Weak::new() is equal to any other pointer, including pointers also created via Weak::new(). That is what the documentations currently says.
There was a problem hiding this comment.
The way the documentation is written implies that you can take a Weak::new and somehow turn it into something that's not a dangling pointer. For example some fn(self: &mut Weak<T>). Assignment and mem::replace don't count.
There was a problem hiding this comment.
Sorry @jethrogb I still don't really see your point. You keep mentioning "dangling pointer", but the public documentation doesn't. Furthermore you can turn a variable with the current value of Weak::new into something else by using plain assignment. That is also possible in any function that takes &mut Weak<T>, so I don't get why that doesn't "count".
There was a problem hiding this comment.
"dangling pointer" and Weak::new are equivalent.
Furthermore you can turn a variable with the current value of Weak::new into something else by using plain assignment.
Dropping the old value and moving something new in its place is not the same as changing the value.
There was a problem hiding this comment.
@jethrogb "dangling pointer" and Weak::new are not equivalent. That is only how the current API is implementated. Previous versions of Rust (not sure what version changed it) actually allocated on the heap when calling Weak::new. A change was made to not allocate by default for most (collection) types, including Weak. However this happend without a change in the API. This means we could go back to that implementation and then Weak::new and "dangling pointer" aren't equivalent anymore.
But maybe someone else can can help here, this discussion is going off the rails and we're clearing not understanding each other.
There was a problem hiding this comment.
@jethrogb "dangling pointer" and Weak::new are not equivalent. That is only how the current API is implementated. Previous versions of Rust (not sure what version changed it) actually allocated on the heap when calling Weak::new. A change was made to not allocate by default for most (collection) types, including Weak. However this happend without a change in the API. This means we could go back to that implementation and then Weak::new and "dangling pointer" aren't equivalent anymore.
Ok fine, I was using them equivalently, but I meant Weak::new. This point is completely orthogonal to my main point about the phrasing of the documentation.
There was a problem hiding this comment.
When using
Weak::newtheWeakis never equal to any otherWeakuntil it points to actual data
The "until it points to actual data" part clearly implies you can take a Weak::new and somehow change it to a value that is real. Dropping the old value and moving something new in its place is not the same as changing the value.
|
I disagree with the handling of the dangling case. Suppose we had an fn get(a: &AtomicWeak<i32>) -> Arc<i32> {
loop {
let weak = a.load();
match weak.upgrade() {
Some(arc) => return arc,
None => {
let arc = Arc::new(0);
let prev = a.compare_and_swap(&weak, Arc::downgrade(&arc));
if Weak::ptr_eq(&prev, &weak) {
return arc;
}
}
}
}
}With the current behavior of |
|
So what should the handling be then? Are dangling weaks equal to each other? To all pointers? |
|
@sfackler Ah, I misunderstood by thinking "dangling weak" means "weak with strong_count = 0". :( Never mind then, the current behavior seems good to me. |
|
@stjepang but that still doesn't make you example work, does it? I mean you can remove the if statement and upgrade on the next loop, but that does mean some more atomic instructions. |
|
It feels surprising that |
|
@Thomasdezeeuw Hmmm, right... if @SimonSapin also has a good point. I think there are two ways to interpret a dangling
|
|
Btw, a small nit: in the docs for |
|
@SimonSapin I agree that it is surprising, but I went of the docs from
Since But if everybody agress that they should be equal to one another then I'm willing to change it. |
|
I don't think they should be equal. |
|
@jethrogb But do you think a dangling pointer should at least be equal to itself? E.g. |
|
Seems reasonable. @bors r+ |
|
📌 Commit 38e21f9 has been approved by |
bors
added
S-waiting-on-bors
Add Weak.ptr_eq I hope the doc tests alone are good enough. We also might want to discuss the dangling pointer case (from `Weak::new()`). Updates rust-lang#55981.
|
@bors rollup |
Add Weak.ptr_eq I hope the doc tests alone are good enough. We also might want to discuss the dangling pointer case (from `Weak::new()`). Updates rust-lang#55981.
Add Weak.ptr_eq I hope the doc tests alone are good enough. We also might want to discuss the dangling pointer case (from `Weak::new()`). Updates rust-lang#55981.
Add Weak.ptr_eq I hope the doc tests alone are good enough. We also might want to discuss the dangling pointer case (from `Weak::new()`). Updates rust-lang#55981.
Add Weak.ptr_eq I hope the doc tests alone are good enough. We also might want to discuss the dangling pointer case (from `Weak::new()`). Updates rust-lang#55981.
Rollup of 15 pull requests Successful merges: - #51753 (Document `From` implementations) - #55563 (Improve no result found sentence in doc search) - #55987 (Add Weak.ptr_eq) - #56119 (Utilize `?` instead of `return None`.) - #56372 (Refer to the second borrow as the "second borrow" in E0501.rs) - #56388 (More MIR borrow check cleanup) - #56424 (Mention raw-ident syntax) - #56452 (Remove redundant clones) - #56456 (Handle existential types in dead code analysis) - #56466 (data_structures: remove tuple_slice) - #56476 (Fix invalid line number match) - #56497 (cleanup: remove static lifetimes from consts in libstd) - #56498 (Fix line numbers display) - #56523 (Added a bare-bones eslint config (removing jslint)) - #56538 (Use inner iterator may_have_side_effect for Cloned) Failed merges: r? @ghost
|
what is the reason for |
|
I just matched the method on |
make `Weak::ptr_eq`s into methods This makes the `Weak::ptr_eq`s associated function into methods. There's no reason for methods on `Weak`s to be associated functions, as there is no `Dered` thus no possibility of a collision. Also: methods can be called using the associated function syntax. follow up on rust-lang#55987 [Tracking issue for weak_ptr_eq](rust-lang#55981)
… r=RalfJung Stabilise weak_ptr_eq Implemented in rust-lang#55987. Closes rust-lang#55981.
… r=RalfJung Stabilise weak_ptr_eq Implemented in rust-lang#55987. Closes rust-lang#55981.
I hope the doc tests alone are good enough.
We also might want to discuss the dangling pointer case (from
Weak::new()).Updates #55981.