-
Notifications
You must be signed in to change notification settings - Fork 12.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Impl task::Wake for Fn #75793
Impl task::Wake for Fn #75793
Conversation
r? @sfackler (rust_highfive has picked a reviewer for you, use r? to override) |
previous discussion: #70764 |
r? @KodrAus |
Thanks for the PR @a1phyr! I'm a bit uncomfortable adding such a broad blanket implementation here, especially since those prior art crates you've linked to seem to work just fine. cc @withoutboats do you have any thoughts on this? |
This is interesting as a solution to It should work because Fn is fundamental, so we can add these sort of blanket impls without conflicting with other impls. If we want to support a conversion from a closure to a waker in std, this seems like the way to do it. However, I'd also note that |
I think I'd personally be a bit more comfortable with something like |
If we do decide to do this, though, it needs to land in the same release as #74304 since |
@sfackler Another example where we do this is the Pattern trait: https://doc.rust-lang.org/std/str/pattern/trait.Pattern.html#impl-Pattern%3C%27a%3E-5 I believe it isn't true that this would not be backward incompatible to add later.. fundamental means something different on traits than on data types; but it also hurts my brains to think about fundamental so I might be wrong. |
Ah yeah that is a good point, though I feel the Wake case is a bit more similar to Iterator than Pattern. Patterns are (almost?) always just passed directly to methods as immediate arguments, where Wake/Iterator more commonly sit around as "persistent" values. |
Which implementation are you talking about ? Both the Anyway, I now think that a |
Regarding the However, it has nothing to do with |
Thanks for the PR @a1phyr! @rfcbot fcp close I think we should not add a blanket impl of So instead of writing this: use std::{sync::Arc, task::Waker};
let waker = Waker::from(Arc::new(|| {
println!("Woken !");
})); You would instead write this: use std::task::Waker;
let waker = Waker::from_fn(|| {
println!("Woken !");
}); Since we already have a completed FCP for the |
Team member @KodrAus has proposed to close this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
Add a blanket
alloc::task::Wake
impl forF: Fn() + Send + Sync + 'static
.Motivation
The goal of the
Wake
trait is to ease the creation ofWaker
s (especially by removing the need ofunsafe
). This proposal goes further by offering to create them from closures :Prior art
waker_fn
async_task::waker_fn
Alternatives
Waker::from_fn
function to provide equivalent functionnality without using traitWake
.