Skip to content
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

Implement task::Wake for Inner #18

Merged
merged 4 commits into from
Oct 8, 2023
Merged

Conversation

dfireBird
Copy link
Contributor

@dfireBird dfireBird commented Oct 7, 2023

Fixes #17.

Please suggest any changes necessary.

Copy link
Member

@notgull notgull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To finish the issue, you would also need something on Unparker to expose this functionality in the API. Something like:

impl From<Unparker> for Waker {
    fn from(up: Unparker) -> Self {
        Waker::from(up.inner)
    }
}

src/lib.rs Outdated Show resolved Hide resolved
@dfireBird
Copy link
Contributor Author

It seems with the implementation of std::task::Wake, the msrv needs to bumped up to 1.51

Also I can't seem to run that test with features loom in my local? Do you know any reason why?

@notgull
Copy link
Member

notgull commented Oct 7, 2023

It seems with the implementation of std::task::Wake, the msrv needs to bumped up to 1.51

Can you bump it in this PR? Just change the version in Cargo.toml and ci.yml from 1.39 to 1.51.

Also I can't seem to run that test with features loom in my local? Do you know any reason why?

You can run loom tests like this:

$ RUSTFLAGS="--cfg loom" cargo test --test loom --features loom

It seems that we can't use loom::sync::Arc in our Waker implementation. Just remove the conditional loom::sync::Arc import and use std::sync::Arc unconditionally, I don't think it matters.

@dfireBird
Copy link
Contributor Author

Can you bump it in this PR? Just change the version in Cargo.toml and ci.yml from 1.39 to 1.51.

Sure, I just want to let you know before doing it actually.

It seems that we can't use loom::sync::Arc in our Waker implementation. Just remove the conditional loom::sync::Arc import and use std::sync::Arc unconditionally, I don't think it matters.

Oh okay, I will make this change then.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@dfireBird
Copy link
Contributor Author

Do you want me to amend the commits into one? The diff is small enough to be a single commit.

@notgull
Copy link
Member

notgull commented Oct 8, 2023

That's okay, I'll just squash it when I review it tomorrow tonight

Copy link
Member

@notgull notgull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@notgull notgull merged commit 0c77e3d into smol-rs:master Oct 8, 2023
@dfireBird dfireBird deleted the impl-wake branch October 8, 2023 08:25
@notgull notgull mentioned this pull request Oct 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add a zero-allocation way to get a Waker from an Unparker
2 participants