Implements the changes for Wakers that are currently discussed in the tracking issue for stabilization #1340

Matthias247 · 2018-11-19T06:33:48Z

This depends on changes to libcore and libstd to work.

See discussion in rust-lang/rfcs#2592 and proposed update in
aturon/rfcs#15

This depends on changes to libcore and libstd to work.

Matthias247 · 2018-11-19T06:33:56Z

I already upload it here, so that people can take a look on the impact of the API changes.
I would especially like to get the parts around FuturesUnordered and LocalWakerRef reviewed, since those were pretty hard to understand. I have changed LocalWakerRef now to produce some very special LocalWaker which doesn't care about refcounts, but which changes it's vtable when it gets cloned. That should be correct.

However I'm not sure why currently local_waker_ref_from_nonlocal is used instead of local_waker_ref.

futures-executor/src/local_pool.rs

futures-executor/src/thread_pool.rs

futures-test/src/task/wake_counter.rs

futures-util/src/future/shared.rs

Centril

Assorted places I think unsafe is unneeded. While skimming this PR I noticed that there are places where unsafe { ... } is used; I think generally, uses of unsafe { ... } should come with a comment containing an informal proof of safety. This is especially true for bits and pieces that are going to be included in the standard library.

Centril · 2018-12-08T03:45:28Z

futures-test/src/task/panic_waker.rs

-    pub fn new() -> Self {
-        Self { _reserved: () }
-    }
+unsafe fn noop(_data: *const()) {


Suggested change

unsafe fn noop(_data: *const()) {

fn noop(_data: *const()) {

You should be able to remove unsafe here because of this coercion between fn and unsafe fn:

fn noop(_data: *const()) {} const PTR: unsafe fn(*const ()) = noop;

Interesting, so there’s a coercion from fn to unsafe fn and from environment-less-closure to fn, but not the combined environment-less-closure to unsafe fn: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=eaec9c9f080c999a55a08dd2184d8669. Has there been any discussion of adding the latter?

I think that's just due to the lack of transitive coercions in general. It is not particular to this case AFAIK.

We generally accept PRs for these without FCP since the original RFC specified that we should include all transitive coercions, but there isn't a mechanism in the compiler for that today.

Centril · 2018-12-08T03:46:23Z

futures-test/src/task/panic_waker.rs

-#[derive(Debug)]
-pub struct PanicWake {
-    _reserved: (),
+unsafe fn noop_clone(_data: *const()) -> RawWaker {


Suggested change

unsafe fn noop_clone(_data: *const()) -> RawWaker {

fn noop_clone(_data: *const()) -> RawWaker {

ditto

Centril · 2018-12-08T03:47:02Z

futures-test/src/task/panic_waker.rs

-    fn default() -> Self {
-        Self::new()
-    }
+unsafe fn wake_panic(_data: *const()) {


Suggested change

unsafe fn wake_panic(_data: *const()) {

fn wake_panic(_data: *const()) {

ditto

Centril · 2018-12-08T03:47:12Z

futures-test/src/task/panic_waker.rs

-    fn wake(_arc_self: &Arc<Self>) {
-        panic!("should not be woken")
-    }
+unsafe fn noop_into_waker(_data: *const()) -> Option<RawWaker> {


Suggested change

unsafe fn noop_into_waker(_data: *const()) -> Option<RawWaker> {

fn noop_into_waker(_data: *const()) -> Option<RawWaker> {

ditto

Centril · 2018-12-08T03:49:24Z

futures-util/src/task/local_waker_ref.rs

    LocalWakerRef::new(local_waker)
 }
+
+unsafe fn noop(_data: *const()) {


Suggested change

unsafe fn noop(_data: *const()) {

fn noop(_data: *const()) {

ditto

Centril · 2018-12-08T03:50:49Z

futures-util/src/task/noop_waker.rs

-#[derive(Debug)]
-struct NoopWake {
-    _reserved: (),
+unsafe fn noop_clone(_data: *const()) -> RawWaker {


Suggested change

unsafe fn noop_clone(_data: *const()) -> RawWaker {

fn noop_clone(_data: *const()) -> RawWaker {

ditto

Centril · 2018-12-08T03:51:00Z

futures-util/src/task/noop_waker.rs

-    unsafe fn drop_raw(&self) {}
-
-    unsafe fn wake(&self) {}
+unsafe fn noop(_data: *const()) {


Suggested change

unsafe fn noop(_data: *const()) {

fn noop(_data: *const()) {

ditto

Centril · 2018-12-08T03:51:18Z

futures-util/src/task/noop_waker.rs

-fn noop_unsafe_wake() -> NonNull<dyn UnsafeWake> {
-    static mut INSTANCE: NoopWake = NoopWake { _reserved: () };
-    unsafe { NonNull::new_unchecked(&mut INSTANCE as *mut dyn UnsafeWake) }
+unsafe fn noop_into_waker(_data: *const()) -> Option<RawWaker> {


Suggested change

unsafe fn noop_into_waker(_data: *const()) -> Option<RawWaker> {

fn noop_into_waker(_data: *const()) -> Option<RawWaker> {

Nemo157 · 2019-02-20T23:58:07Z

@Matthias247 can be closed now?

Matthias247 · 2019-02-21T00:11:58Z

Merged in a different CR

Implements the changes for Wakers that are currently discussed.

5bd4572

This depends on changes to libcore and libstd to work.

Matthias247 changed the title ~~Implements the changes for Wakers that are currently discussed.~~ Implements the changes for Wakers that are currently discussed in the tracking issue for stabilization Nov 19, 2018

Fix typo

8b25936

cramertj reviewed Nov 19, 2018

View reviewed changes

futures-executor/src/local_pool.rs Outdated Show resolved Hide resolved

cramertj reviewed Nov 19, 2018

View reviewed changes

futures-executor/src/thread_pool.rs Outdated Show resolved Hide resolved

cramertj reviewed Nov 19, 2018

View reviewed changes

futures-test/src/task/wake_counter.rs Outdated Show resolved Hide resolved

cramertj reviewed Nov 19, 2018

View reviewed changes

futures-util/src/future/shared.rs Outdated Show resolved Hide resolved

Remove unnecessary wake_local implementations

51f8821

Centril reviewed Dec 8, 2018

View reviewed changes

Matthias247 closed this Feb 21, 2019

Matthias247 deleted the waker_changes branch February 21, 2019 00:12

	unsafe fn noop_clone(_data: *const()) -> RawWaker {
	fn noop_clone(_data: *const()) -> RawWaker {

	unsafe fn wake_panic(_data: *const()) {
	fn wake_panic(_data: *const()) {

	unsafe fn noop_into_waker(_data: *const()) -> Option<RawWaker> {
	fn noop_into_waker(_data: *const()) -> Option<RawWaker> {

Implements the changes for Wakers that are currently discussed in the tracking issue for stabilization #1340

Implements the changes for Wakers that are currently discussed in the tracking issue for stabilization #1340

Uh oh!

Conversation

Matthias247 commented Nov 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Matthias247 commented Nov 19, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Centril left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nemo157 Dec 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nemo157 commented Feb 20, 2019

Uh oh!

Matthias247 commented Feb 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Matthias247 commented Nov 19, 2018 •

edited

Loading

Nemo157 Dec 9, 2018 •

edited

Loading