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

cmp: min/max is inconsistent with partial_min/max on certain edge cases #23687

Closed
HeroesGrave opened this issue Mar 25, 2015 · 4 comments · Fixed by #23878
Closed

cmp: min/max is inconsistent with partial_min/max on certain edge cases #23687

HeroesGrave opened this issue Mar 25, 2015 · 4 comments · Fixed by #23878

Comments

@HeroesGrave
Copy link
Contributor

cmp::partial_min(a, b) returns a if a <= b, while cmp::min(a, b) return a if a < b.
Same story with partial_/max.

This will cause unexpected behaviour when a < b == a > b == false, but a != b (ie: ord treats them as equal, but they aren't equal). For simple types this isn't an issue because if ord treats them as equal they are usually equal. However, for more complex types, partial_min(a, b) != min(a, b) etc. which will cause subtle errors and a lot of pain tracking down the cause.

The behaviour of the partial_ variants in this case seems more correct to me. When the two arguments are equal, it is more intuitive to return the first.

Not sure if this needs an RFC to go through.

@Ryman
Copy link
Contributor

Ryman commented Mar 25, 2015

Additionally, there is inconsistency between cmp::min/max and foo.iter().min/max as can be seen here: http://is.gd/aSAlz0

It should be mentioned that the author of max in STL considers their implementation to be wrong, as it returns a when a == b which loses the stability property. Which, when used for sorting can lead to unnecessary swaps.

It would be nice to get this consistent across the board.

cc @aturon

@aturon
Copy link
Member

aturon commented Mar 25, 2015

I agree that these should be made consistent; I don't think it requires a full RFC to do so.

I had also heard that bit of wisdom from the STL, suggesting that the cmp::min behavior is preferred. I don't personally have a strong preference here, other than for consistency.

cc @alexcrichton

@Ryman
Copy link
Contributor

Ryman commented Mar 25, 2015

To be explicit: The hindsight from STL as I understand it is that cmp::min should return the first argument if they're equal, while cmp::max should return the second argument if they're equal.

Currently our cmp::max is correct while cmp::min is wrong.

@alexcrichton
Copy link
Member

Yeah I also think that this is fine to change without an RFC, and I also don't mind going with what the accepted way should be.

alexcrichton added a commit to alexcrichton/rust that referenced this issue Mar 31, 2015
`min`-like functions now return the leftmost element/input for equal elements.
`max`-like return the rightmost.

Closes rust-lang#23687.

cc @HeroesGrave, @aturon, @alexcrichton
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 a pull request may close this issue.

5 participants