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

Bring 0.3 branch up to date with new RFC #986

Closed
wants to merge 7 commits into from

Conversation

aturon
Copy link
Member

@aturon aturon commented Apr 25, 2018

This PR updates the 0.3 work to match the new RFC.

The approach here keeps the Future trait with an Error type, but uses the new Poll type; an alias PollResult<T, E> = Poll<Result<T, E>> is defined.

The combinators for Future are added back to this branch and updated with the Poll changes, but otherwise are as in 0.2. We'll need follow up with changes to bring them into line, e.g. renaming map to map_ok etc.

My plan is to follow this same approach to get the crates back into working condition (including tests), and then circle back to harmonize the combinators.

@aturon aturon requested a review from cramertj April 25, 2018 05:34

/// An `Async` value represents an asychronous computation.
///
/// An `Asyn` is a value that may not have finished computing yet. This kind of
Copy link
Member

Choose a reason for hiding this comment

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

s/Asyn/Async

/// [executor](../futures_core/executor/trait.Executor.html) as a new, independent
/// task that will automatically be `poll`ed to completion.
///
/// # Combinators
Copy link
Member

@cramertj cramertj Apr 25, 2018

Choose a reason for hiding this comment

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

Here and above you mention combinators on Async-- this should link to AsyncExt.

/// This method is a stopgap for a compiler limitation that prevents us from
/// directly inheriting from the `Async` trait; in the future it won't be
/// needed.
fn poll(self: Pin<Self>, cx: &mut task::Context) -> PollResult<Self::Item, Self::Error>;
Copy link
Member

@cramertj cramertj Apr 25, 2018

Choose a reason for hiding this comment

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

This should include into_async so that AsyncResults can be awaited (well, I suppose it could go on AsyncResultExt, but the method should exist somewhere). Also, i think the name of the poll method here should be poll_result so that it doesn't cause ambiguities.

/// Create a future that is immediately ready with a value.
pub fn ready<T>(t: T) -> ReadyFuture<T> {
ReadyFuture(Some(t))
impl<F> IntoFuture for F where F: Future {
Copy link
Member

@cramertj cramertj Apr 25, 2018

Choose a reason for hiding this comment

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

I think you also want an impl<A> IntoFuture for A where A: AsyncResult + Unpin { ... }.

/// [`err`](::future::err) functions.
#[derive(Debug, Clone)]
#[must_use = "futures do nothing unless polled"]
pub struct FutureResult<T, E> {
Copy link
Member

@cramertj cramertj Apr 25, 2018

Choose a reason for hiding this comment

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

Might as well add an impl of Async + Unpin for FutureResult (same goes for all the other Future-specific combinators).

pub fn never_into<T>(self) -> T {
match self {}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Also want Async for Never.


/// Shorthand for a `Poll<Result<_, _>>` value.
pub type PollResult<T, E> = Poll<Result<T, E>>;

Copy link
Member

Choose a reason for hiding this comment

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

It seems like we should also have a macro here for propagating Pending that works on non-Resulty Polls.

@cramertj
Copy link
Member

The general strategy in this PR takes is to copy all of the combinators over to Future. Is there a reason for organizing it this way, rather than having single combinator types which implement either Async and Future based on their type parameters?

@aturon
Copy link
Member Author

aturon commented May 2, 2018

Closing this out, as I'm taking a different approach now.

@aturon aturon closed this May 2, 2018
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