Smaller await #1159
Smaller await #1159
Conversation
futures-util/src/async_await/join.rs
Outdated
| let mut all_done = true; | ||
| $( | ||
| if $crate::poll!($fut.reborrow()).is_pending() { | ||
| if $crate::future::Future::poll($fut.reborrow(), cx).is_pending() { |
There was a problem hiding this comment.
It's safe to use the method call syntax because we know that it's a MaybeDone future.
There was a problem hiding this comment.
With macros, since they're expanded in a user's crate, you never know what other traits might be in scope that happen to have a conflicting method name. It's much safer to fully qualify the method call, and costs nothing :)
There was a problem hiding this comment.
I'm aware of that. It's fine here because this type is under our control because we define it.
We already do the same with .reborrow() here and with non-macro code everywhere else
There was a problem hiding this comment.
@MajorBreakfast the problem is the user could introduce a new trait that has a poll method and implement that for MaybeDone. That’s highly unlikely, but the only way to guarantee no method resolution errors is to always use expanded function calls.
There was a problem hiding this comment.
Ah, I see. And it's okay for reborrow because it's an inherent method and those have precedence (I just checked that because I was curious)
There was a problem hiding this comment.
Yup, as others said we can't assume that the Future trait is in-scope or that other methods called poll haven't been provided via other traits. .reborrow() is fine because it's an inherent method.
futures-util/src/async_await/join.rs
Outdated
| // `.ok().unwrap()` rather than `.unwrap()` so that we don't introduce | ||
| // an `E: Debug` bound. | ||
| $fut.reborrow().take_output().unwrap().ok().unwrap(), | ||
| )*))) |
There was a problem hiding this comment.
Inconsistent indentation
| // an `E: Debug` bound. | ||
| $fut.reborrow().take_output().unwrap().ok().unwrap(), | ||
| )*)) | ||
| res |
There was a problem hiding this comment.
It's basically:
let res = await!(/* ... */);
resYou could give back the value from the expression above directly
There was a problem hiding this comment.
It's intentionally done this way in order to provide the : Result<_, _> annotation .
futures-util/src/future/mod.rs
Outdated
| @@ -5,7 +5,7 @@ | |||
|
|
|||
| use core::marker::Unpin; | |||
| use core::mem::PinMut; | |||
| use futures_core::future::Future; | |||
| pub use futures_core::future::Future; | |||
There was a problem hiding this comment.
Should be doc hidden.
There was a problem hiding this comment.
And maybe a comment // Reexported for use in macros
There was a problem hiding this comment.
Or even better: Add a futures_core_reexport (doc hidden)
There was a problem hiding this comment.
We shouldn't go overboard with reexports. We should try to only use reexports in our facade and in futures_core.
| &mut $name)); | ||
| if let $crate::core_reexport::task::Poll::Ready(x) = poll_res { | ||
| break __PrivResult::$name(x); | ||
| match $crate::future::Future::poll( |
There was a problem hiding this comment.
$crate::core_reexport::future::Future would also work. It's IMO no different than for Poll where you already do it this way
| { | ||
| $crate::core_reexport::task::Poll::Ready(x) => | ||
| return $crate::core_reexport::task::Poll::Ready(__PrivResult::$name(x)), | ||
| $crate::core_reexport::task::Poll::Pending => {}, |
There was a problem hiding this comment.
Since this branch is empty, you could use if let $crate::...::Poll::Ready(x) = poll {} instead
There was a problem hiding this comment.
I intentionally changed it because I think it's easier to read as a match since all of the paths included are so long.
There was a problem hiding this comment.
I'd have introduced a poll variable. But match is also fine
asyncbodies are huge right now, largely due to rust-lang/rust#52924, but this change makes them quite a bit smaller. I've added tests so that we know when the sizes go up or down in response to changes-- these will be somewhat volatile in response to MIR changes, especially any work on rust-lang/rust#52924, but I think it's good for us to watch and be aware of any changes with respect to the size of the generated structures.