Update to latest nightly, removing LocalWaker and introducing ArcWake #1
Conversation
src/arc_wake.rs
Outdated
| @@ -0,0 +1,78 @@ | |||
| // From htttps://github.com/rust-lang/rustc | |||
There was a problem hiding this comment.
Since I pulled this file from rust-lang/rustc I included their LICENSE-MIT in the top of the file.
There was a problem hiding this comment.
Very good :) However, the license also needs to be shipped with binary releases. We're not hosting any of those here, so technically this repository is fine. But if people use this crate as dependency, they'll almost certainly miss the license if it's not in the LICENSE file.
Could you add the license to the LICENSE file instead, below the BSD license, with a note that it applies to this file?
There was a problem hiding this comment.
The comment about the origin of the file is nice anyway, lets keep that in here.
|
Thanks for the PR! I'll need some time to properly review this, since wakers aren't the easiest thing in the world to do safely. I'd kinda like to avoid the inclusion of |
There was a problem hiding this comment.
I had some more time to review the PR. I believe it indeed restores functionality correctly.
It may actually be possible to map the Waker and Notifier traits to each-other without creating an Arc, but that was already on the TODO list anyway. So lets merge this with Arc, and I'll see it can be done without them later.
Just one reply to your comment regarding the license that we should sort out first.
src/arc_wake.rs
Outdated
| @@ -0,0 +1,78 @@ | |||
| // From htttps://github.com/rust-lang/rustc | |||
There was a problem hiding this comment.
Very good :) However, the license also needs to be shipped with binary releases. We're not hosting any of those here, so technically this repository is fine. But if people use this crate as dependency, they'll almost certainly miss the license if it's not in the LICENSE file.
Could you add the license to the LICENSE file instead, below the BSD license, with a note that it applies to this file?
|
Like this? :) |
|
Oh jeez :P I'm at my work so I can't really do work on fixing this right now. Maybe in the weekend.. |
|
Yeah the license is great like that. But it seems we've been caught up by rust development again :p |
|
I tried to fix it, which ironically seemed to require removing Either way, I made a vtable for Also, my apolagies for pushing to your fork, I always get a bit paranoid when that happens. In case you didn't know, github allows that by default for collaborators of the target repository for a PR (but you can disable it). |
de-vri-es
force-pushed
the
no_local_waker
branch
from
eccf87a to
9fff2cc
Compare
|
|
||
| impl TaskWaker { | ||
| unsafe fn into_raw_waker(task: futures::task::Task) -> RawWaker { | ||
| let data = Box::leak(Box::new(task)) as *const _ as *const (); |
There was a problem hiding this comment.
What's going on with the double as .., is that necessary?
There was a problem hiding this comment.
Yeah, you can't cast directly to a pointer of a different type, but you can cast pointers to pointers of different types. So first it casts to a *const futures::task::Task (abbreviated as *const _), then to a *const ().
|
I'll go ahead and merge. Thanks for the PR, I hadn't even noticed it was broken yet :) |
|
heh, it already didn't compile anymore. Fixed in 940ccd9. |
I didn't really delve into the implications of these changes, I noticed the project no
longer compiled on nightly, and found this PR:
rust-lang/rust#57992
So I basically mapped the new unit tests in there to this project, until
this project compiled again. Then I ran the suite and it passed so
I reckon it at least sort of works. It definitely needs a review
by someone who actually understands the code (hoping that is you ;))