SI-9087 Fix min/max of reversed Double/Float orderings #4253
Conversation
|
LGTM - the behavior looks correct (inasmuch as any of the NaN behavior really feels "correct"), and I think any bincompat issues are ignorable since it's an anonymous class. (FWIW, if "reverse" gets used much, having it create it anew on each call on the implicit object is a bad idea, but that's not related to this fix.) |
There was a problem hiding this comment.
I don't think it makes any practical difference, but why are the args not swapped here?
There was a problem hiding this comment.
I wanted to highlight that we swap the operation, not the args.
There was a problem hiding this comment.
Okay, but then why are the args swapped here? Not that it really matters since practically nobody cares which NaN it is, but both min and max return the first argument's NaN even if the second argument is also NaN.
There was a problem hiding this comment.
I've just changed this one to min(x, y).
|
PLS REBUILD ALL |
|
(kitty-note-to-self: ignore 70583087) |
retronym
force-pushed
the
ticket/9087
branch
2 times, most recently
from
cefa0e1 to
177384f
Compare
As diagnosed by the reporter, we needed additional overrides
due to the way these orderings are implemented.
I've added tests to show other methods and other orderings
are working correctly.
After writing that, I found a scalacheck test related to
NaN handling that also covers `Ordering`. I had to correct
the assertion in the tests of `reverse.{min,max}`.
|
@Ichoran I've pushed a change that updates an incorrect assertion in an existing test. |
|
LGTM still (LEBTM, I guess--looks even better)! |
SI-9087 Fix min/max of reversed Double/Float orderings
4 participants
As diagnosed by the reporter, we needed additional overrides
due to the way these orderings are implemented.
I've added tests to show other methods and other orderings
are working correctly.
Review by @Ichoran