-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Make async generators fused by default #118764
Conversation
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
Out of curiosity, what changed your mind? I sort of liked having them not be fused by default, but I don't have a good reason or enough experience/intuition to really justify either position. |
I guess if generators are already fused by default it makes sense to have the same behavior for async iterators. Code-wise, the PR looks good to me. I'd like to give @rust-lang/wg-async a little more time to express opinions though. |
The existence of
Yeah, I thought the same. |
The general feedback so far has been that fusing by default is the right choice. I'll go ahead and approve this. This feature is experimental so the stakes are pretty low and we can always revisit the decision in the future. @bors r+ |
@bors rollup |
…or, r=eholk Make async generators fused by default I actually changed my mind about this since the implementation PR landed. I think it's beneficial for `async gen` blocks to be "fused" by default -- i.e., for them to repeatedly return `Poll::Ready(None)` -- rather than panic. We have [`FusedStream`](https://docs.rs/futures/latest/futures/stream/trait.FusedStream.html) in futures-rs to represent streams with this capability already anyways. r? eholk cc `@rust-lang/wg-async,` would like to know if anyone else has opinions about this.
…or, r=eholk Make async generators fused by default I actually changed my mind about this since the implementation PR landed. I think it's beneficial for `async gen` blocks to be "fused" by default -- i.e., for them to repeatedly return `Poll::Ready(None)` -- rather than panic. We have [`FusedStream`](https://docs.rs/futures/latest/futures/stream/trait.FusedStream.html) in futures-rs to represent streams with this capability already anyways. r? eholk cc ``@rust-lang/wg-async,`` would like to know if anyone else has opinions about this.
…kingjubilee Rollup of 8 pull requests Successful merges: - rust-lang#118198 (coverage: Use `SpanMarker` to improve coverage spans for `if !` expressions) - rust-lang#118512 (Add tests related to normalization in implied bounds) - rust-lang#118610 (update target feature following LLVM API change) - rust-lang#118666 (coverage: Simplify the heuristic for ignoring `async fn` return spans) - rust-lang#118737 (Extend tidy alphabetical checking to `tests/`.) - rust-lang#118756 (use bold magenta instead of bold white for highlighting) - rust-lang#118762 (Some more minor `async gen`-related nits) - rust-lang#118764 (Make async generators fused by default) r? `@ghost` `@rustbot` modify labels: rollup
…kingjubilee Rollup of 7 pull requests Successful merges: - rust-lang#118198 (coverage: Use `SpanMarker` to improve coverage spans for `if !` expressions) - rust-lang#118512 (Add tests related to normalization in implied bounds) - rust-lang#118610 (update target feature following LLVM API change) - rust-lang#118666 (coverage: Simplify the heuristic for ignoring `async fn` return spans) - rust-lang#118737 (Extend tidy alphabetical checking to `tests/`.) - rust-lang#118762 (Some more minor `async gen`-related nits) - rust-lang#118764 (Make async generators fused by default) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#118764 - compiler-errors:fused-async-iterator, r=eholk Make async generators fused by default I actually changed my mind about this since the implementation PR landed. I think it's beneficial for `async gen` blocks to be "fused" by default -- i.e., for them to repeatedly return `Poll::Ready(None)` -- rather than panic. We have [`FusedStream`](https://docs.rs/futures/latest/futures/stream/trait.FusedStream.html) in futures-rs to represent streams with this capability already anyways. r? eholk cc ```@rust-lang/wg-async,``` would like to know if anyone else has opinions about this.
I actually changed my mind about this since the implementation PR landed. I think it's beneficial for
async gen
blocks to be "fused" by default -- i.e., for them to repeatedly returnPoll::Ready(None)
-- rather than panic.We have
FusedStream
in futures-rs to represent streams with this capability already anyways.r? eholk
cc @rust-lang/wg-async, would like to know if anyone else has opinions about this.