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 Future for SmallBox<F> where F: Future #39

Merged
merged 5 commits into from
Dec 23, 2024

Conversation

PonasKovas
Copy link
Contributor

@PonasKovas PonasKovas commented Dec 2, 2024

Previously discussed in #38

Additionally I ran cargo fmt in the repository.

I am pretty certain that this is safe and sound, but if you want to discuss this further I will gladly do so.

Closes #38

@andylokandy
Copy link
Owner

would you like to add an example demonstrating how SmallBox can be uesd in async context?

@PonasKovas
Copy link
Contributor Author

You mean demonstrate to you, or add it to the repository somewhere? To the readme?

@PonasKovas
Copy link
Contributor Author

PonasKovas commented Dec 2, 2024

It's really pretty simple, similar to the std Box in user-facing code, but doesn't need to be pinned with Box::pin

Here's a simple example:

async fn foo() {
    let fut = async {
        sleep(Duration::from_secs(1)).await;
        println!("finished");
    };

    let boxed_fut: SmallBox<_, S1> = SmallBox::new(fut);

    let erased = boxed_fut as SmallBox<dyn Future<Output = ()>, S1>;

    erased.await;
}

@andylokandy
Copy link
Owner

Looks good to me. And would you like to add the async example into the unit tests, to ensure the impl Future works?

@PonasKovas
Copy link
Contributor Author

@andylokandy I could, but then I would need to pull in some dev-dependencies, either some framework like tokio or futures crate. Tell me if that's okay.

Personally I don't think this needs any testing since this feature is pretty much a compile time thing.

@andylokandy
Copy link
Owner

andylokandy commented Dec 22, 2024

is pollster sufficient?

@PonasKovas
Copy link
Contributor Author

Yes. I will do it with pollster then

@andylokandy
Copy link
Owner

There is a conflict in the commit. I'll merge once it's resolved.

@PonasKovas
Copy link
Contributor Author

PonasKovas commented Dec 22, 2024

Hmm probably pollster doesn't support this MSRV

Yep, pollster's MSRV is 1.69, while this project is 1.56

@PonasKovas
Copy link
Contributor Author

I can switch to futures crate, it's MSRV is also 1.56

@andylokandy andylokandy merged commit edd5464 into andylokandy:master Dec 23, 2024
9 checks passed
@andylokandy
Copy link
Owner

Released 0.8.6 #41

@PonasKovas PonasKovas deleted the future branch December 23, 2024 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dyn Future
2 participants