-
Notifications
You must be signed in to change notification settings - Fork 13.2k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Add Arc::new_cyclic #75505
Add Arc::new_cyclic #75505
Conversation
library/alloc/src/sync.rs
Outdated
/// # Examples | ||
/// ``` | ||
/// #![feature(arc_new_cyclic)] | ||
/// #![allow(dead_code)] |
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.
/// #![allow(dead_code)] | |
/// # #![allow(dead_code)] |
library/alloc/src/sync.rs
Outdated
@@ -328,6 +328,79 @@ impl<T> Arc<T> { | |||
Self::from_inner(Box::leak(x).into()) | |||
} | |||
|
|||
/// Constructs a new `Arc<T>` using a weak reference to itself. Attempting | |||
/// to upgrade the weak reference before this function returns will result |
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.
I think the indentation for docs is off starting here.
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.
That's odd, is this not just cherry-picking the commits of #72443?
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.
cherry picking took a while due to some conflicts since this pr was small, I pasted the contents directly into respectively files. I ran tidy again now which would have fixed this issues
library/alloc/src/sync.rs
Outdated
/// let foo = Arc::new_cyclic(|me| Foo { | ||
/// me: me.clone(), | ||
/// }); | ||
/// ``` |
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.
What is the use case for this example? I don't understand by just looking at the example, maybe we should provide a more realistic use case?
@@ -1608,8 +1682,11 @@ impl<T: ?Sized> Weak<T> { | |||
abort(); | |||
} | |||
|
|||
// Relaxed is valid for the same reason it is on Arc's Clone impl | |||
match inner.strong.compare_exchange_weak(n, n + 1, Relaxed, Relaxed) { | |||
// Relaxed is fine for the failure case because we don't have any expectations about the new state. |
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.
Isn't this comment too long?
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.
I find it helpful so +1
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.
As noted before, a similar API should be added to Rc
.
library/alloc/src/sync.rs
Outdated
/// }); | ||
/// ``` | ||
#[inline] | ||
#[unstable(feature = "arc_new_cyclic", issue = "none")] |
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.
#[unstable(feature = "arc_new_cyclic", issue = "none")] | |
#[unstable(feature = "arc_new_cyclic", issue = "75861")] |
I've opened up #75861 as a tracking issue with a note about adding a similar API for r=me with the current suggested edits added. |
@bors r=KodrAus |
📌 Commit c26a8d5 has been approved by |
⌛ Testing commit c26a8d5 with merge 9288641c15b77a6a4651e2d4ca5a6583d0e19d92... |
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 |
💔 Test failed - checks-actions |
Not sure what this is:
But let's hope it's a spurious network issue. @bors retry |
☀️ Test successful - checks-actions, checks-azure |
Rework of #72443
References #75861
cc @Diggsey @RalfJung
r? @KodrAus