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 min specialisation for RangeFrom #47362

Closed
wants to merge 1 commit into from

Conversation

varkor
Copy link
Member

@varkor varkor commented Jan 11, 2018

Calling min on RangeFrom currently causes an infinite loop. Although other methods such as max also result in an infinite loop, it is strictly incorrect in the case of min. Adding a specialisation fixes this.

Separated from #47180 because this technically changes behaviour; it’s not just an optimisation, so it’s a little different.

r? @alexcrichton

Calling `min` on `RangeFrom` currently causes an infinite loop.
Although other methods such as `max` also result in an infinite loop,
it is strictly incorrect in the case of `min`. Adding a specialisation
fixes this.

Separated from rust-lang#47180 because
this technically changes behaviour; it’s not just an optimisation, so
it’s a little different.
@kennytm kennytm added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 11, 2018
@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Jan 11, 2018

📌 Commit 554fbc2 has been approved by alexcrichton

@ollie27
Copy link
Member

ollie27 commented Jan 11, 2018

If this is valid implementation of min then should Repeat also implement min, max etc. like this?

@varkor
Copy link
Member Author

varkor commented Jan 12, 2018

@ollie27 — I was waiting for this change to be approved before correcting the behavior of other infinite iterators, but you're right: I have a PR for Repeat and Cycle here now.

@scottmcm
Copy link
Member

This is an insta-stable behavior change, not just an optimization, so I really think it needs an FCP.

It results in min() and min_by_key(|x| x) doing different things, which feels really weird to me.

@sfackler
Copy link
Member

I am also not totally on board with this change.

@alexcrichton
Copy link
Member

@bors: r-

er sorry I didn't realize this was a behavior change!

@varkor mind detaling why you think this behavior change is ok? e.g. enabling this on this iterator?

@varkor
Copy link
Member Author

varkor commented Jan 12, 2018

@alexcrichton: Sorry, I should have made it clearer! It makes sense to have this discussion in one place, so maybe let's move it to #47370 where it's already going on.

@alexcrichton
Copy link
Member

Sure yeah sounds good to me!

@varkor
Copy link
Member Author

varkor commented Jan 13, 2018

Better to close this one for now as per the discussions in #47370. Sorry for the confusion!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants