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

futures_util::stream::SelectAll::push should use &self. #2293

Merged
merged 1 commit into from
Feb 26, 2021

Conversation

archseer
Copy link
Contributor

FuturesUnordered::push bound is &self, so SelectAll should be able to use the same bound.

FuturesUnordered::push bound is &self, so SelectAll should be able to use the same bound.
@taiki-e taiki-e added A-stream Area: futures::stream S-blocked Status: Blocked on something else labels Dec 22, 2020
@taiki-e taiki-e added this to the futures-0.4 milestone Dec 22, 2020
Copy link
Member

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

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

Thanks! As this is a breaking change, I'll merge this when master switches to 0.4-alpha.

@archseer
Copy link
Contributor Author

Is it a breaking change though? It's just loosening the bound from mut to ref. Should be similar to going from FnMut to Fn.

@taiki-e
Copy link
Member

taiki-e commented Dec 23, 2020

impl Fn type is guaranteed to be impl FnMut, but fn(&mut T, ..) and fn(&T, ..) are different types.
Given that SelectAll::push has multiple arguments (self + item), I think this is very unlikely to cause actual breakage, but I tend to consider this as a breaking change. (Unfortunately, RFC 1105 has no explicit mention of changes like this.)

@taiki-e taiki-e merged commit 74ff5a1 into rust-lang:master Feb 26, 2021
exrook pushed a commit to exrook/futures-rs that referenced this pull request Apr 5, 2021
FuturesUnordered::push bound is &self, so SelectAll should be able to use the same bound.
exrook pushed a commit to exrook/futures-rs that referenced this pull request Apr 5, 2021
FuturesUnordered::push bound is &self, so SelectAll should be able to use the same bound.
@taiki-e taiki-e removed the S-blocked Status: Blocked on something else label Apr 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-stream Area: futures::stream
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants