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

Add try_from constructor #16

Merged
merged 2 commits into from
Sep 2, 2022
Merged

Add try_from constructor #16

merged 2 commits into from
Sep 2, 2022

Conversation

eholk
Copy link
Contributor

@eholk eholk commented Sep 2, 2022

This gives a version that does not panic if the future does not fit which allows programs to recover, such as by boxing the future instead.

This gives a version that does not panic if the future does not fit
which allows programs to recover, such as by boxing the future instead.
/// `Err` with the argument `future` returned to you.
///
/// If we cannot satisfy the alignment requirements for `F`, this function will panic.
pub fn try_from<F>(future: F) -> Result<Self, F>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

An alternative that I'll mention for discussion here is to have this return Result<Self, (StackFutureError, F)>, where StackFutureError is something like:

enum StackFutureError {
    BadAlignment { required: usize, available: usize },
    TooSmall { required: usize, available: usize },
}

That would give a lot more information, but I'm not sure it's useful.

I opted for returning just F because most of the time that's going to be what you want and it avoids having to unpack a tuple on failure cases. The new implementation of from shows how to use this and that you can still find the required size and alignment pretty easily using things like mem::size_of_val.

That said, changing this would be semver-breaking in the future, so it's probably better to discuss which way we want to go now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is reasonable.

Tangentially related: as discussed offline, it should be possible to sidestep the alignment panic entirely through the use of something like https://docs.rs/elain/0.3.0/elain/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about the alignment issue some more, and one problem is that the alignment on StackFuture needs to be compatible with all futures it might wrap, the same way it needs to be at least as large as all futures it might wrap. This means the alignment would have to become part of the type, so instead of StackFuture<T, STACK_SIZE> we'd have StackFuture<T, STACK_SIZE, ALIGNMENT>. The elain crate would let us actually implement that, but this seems like a pretty big ergonomic hit to fix a problem we haven't seen in practice yet.

If we add the fallback-on-box path (which I'm hopefully going to get done this afternoon), that does give us another path here too: we can box the future if the alignment of the future isn't compatible with the StackFuture. That'd be a behavior change, since mismatched alignment would no longer be a panic, but I don't think that's technically semver breaking.

@eholk eholk requested a review from daprilik September 2, 2022 19:53
Copy link
Collaborator

@daprilik daprilik left a comment

Choose a reason for hiding this comment

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

LGTM, aside from the potential semver hazard and some doc nits

src/lib.rs Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
/// `Err` with the argument `future` returned to you.
///
/// If we cannot satisfy the alignment requirements for `F`, this function will panic.
pub fn try_from<F>(future: F) -> Result<Self, F>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is reasonable.

Tangentially related: as discussed offline, it should be possible to sidestep the alignment panic entirely through the use of something like https://docs.rs/elain/0.3.0/elain/

@eholk eholk merged commit 577c7a8 into microsoft:main Sep 2, 2022
@eholk eholk deleted the try_from branch September 2, 2022 23:55
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.

2 participants