Skip to content

Commit 460bbc1

Browse files
khernyokhernyo
committed
fixes scala#5104 and related NaN ordering inconsistencies
The bug was caused by the inconsistency between j.l.Math.min() and j.l.Double.compareTo() wrt NaN (j.l.Math.min() considers NaN to be less than any other value while j.l.Double.compareTo() says it's greater...) The fix changes Ordering.{FloatOrdering,DoubleOrdering) to base it's results on primitive comparisons and math.{min,max} instead of j.l.{Float,Double}.compareTo()
1 parent 5ee9a14 commit 460bbc1

File tree

2 files changed

+170
-0
lines changed

2 files changed

+170
-0
lines changed

‎src/library/scala/math/Ordering.scala

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -262,12 +262,52 @@ object Ordering extends LowPriorityOrderingImplicits {
262262
implicit object Long extends LongOrdering
263263

264264
trait FloatOrdering extends Ordering[Float] {
265+
outer =>
266+
265267
def compare(x: Float, y: Float) = java.lang.Float.compare(x, y)
268+
269+
override def lteq(x: Float, y: Float): Boolean = x <= y
270+
override def gteq(x: Float, y: Float): Boolean = x >= y
271+
override def lt(x: Float, y: Float): Boolean = x < y
272+
override def gt(x: Float, y: Float): Boolean = x > y
273+
override def equiv(x: Float, y: Float): Boolean = x == y
274+
override def max(x: Float, y: Float): Float = math.max(x, y)
275+
override def min(x: Float, y: Float): Float = math.min(x, y)
276+
277+
override def reverse: Ordering[Float] = new FloatOrdering {
278+
override def reverse = outer
279+
override def compare(x: Float, y: Float) = outer.compare(y, x)
280+
281+
override def lteq(x: Float, y: Float): Boolean = outer.lteq(y, x)
282+
override def gteq(x: Float, y: Float): Boolean = outer.gteq(y, x)
283+
override def lt(x: Float, y: Float): Boolean = outer.lt(y, x)
284+
override def gt(x: Float, y: Float): Boolean = outer.gt(y, x)
285+
}
266286
}
267287
implicit object Float extends FloatOrdering
268288

269289
trait DoubleOrdering extends Ordering[Double] {
290+
outer =>
291+
270292
def compare(x: Double, y: Double) = java.lang.Double.compare(x, y)
293+
294+
override def lteq(x: Double, y: Double): Boolean = x <= y
295+
override def gteq(x: Double, y: Double): Boolean = x >= y
296+
override def lt(x: Double, y: Double): Boolean = x < y
297+
override def gt(x: Double, y: Double): Boolean = x > y
298+
override def equiv(x: Double, y: Double): Boolean = x == y
299+
override def max(x: Double, y: Double): Double = math.max(x, y)
300+
override def min(x: Double, y: Double): Double = math.min(x, y)
301+
302+
override def reverse: Ordering[Double] = new DoubleOrdering {
303+
override def reverse = outer
304+
override def compare(x: Double, y: Double) = outer.compare(y, x)
305+
306+
override def lteq(x: Double, y: Double): Boolean = outer.lteq(y, x)
307+
override def gteq(x: Double, y: Double): Boolean = outer.gteq(y, x)
308+
override def lt(x: Double, y: Double): Boolean = outer.lt(y, x)
309+
override def gt(x: Double, y: Double): Boolean = outer.gt(y, x)
310+
}
271311
}
272312
implicit object Double extends DoubleOrdering
273313

‎test/files/scalacheck/nan-ordering.scala

Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
1+
import org.scalacheck._
2+
import Gen._
3+
import Prop._
4+
5+
object Test extends Properties("NaN-Ordering") {
6+
7+
val specFloats: Gen[Float] = oneOf(
8+
Float.MaxValue,
9+
Float.MinPositiveValue,
10+
Float.MinValue,
11+
Float.NaN,
12+
Float.NegativeInfinity,
13+
Float.PositiveInfinity,
14+
-0.0f,
15+
+0.0f
16+
)
17+
18+
property("Float min") = forAll(specFloats, specFloats) { (d1, d2) => {
19+
val mathmin = math.min(d1, d2)
20+
val numericmin = d1 min d2
21+
mathmin == numericmin || mathmin.isNaN && numericmin.isNaN
22+
}
23+
}
24+
25+
property("Float max") = forAll(specFloats, specFloats) { (d1, d2) => {
26+
val mathmax = math.max(d1, d2)
27+
val numericmax = d1 max d2
28+
mathmax == numericmax || mathmax.isNaN && numericmax.isNaN
29+
}
30+
}
31+
32+
val numFloat = implicitly[Numeric[Float]]
33+
34+
property("Float lt") = forAll(specFloats, specFloats) { (d1, d2) => numFloat.lt(d1, d2) == d1 < d2 }
35+
36+
property("Float lteq") = forAll(specFloats, specFloats) { (d1, d2) => numFloat.lteq(d1, d2) == d1 <= d2 }
37+
38+
property("Float gt") = forAll(specFloats, specFloats) { (d1, d2) => numFloat.gt(d1, d2) == d1 > d2 }
39+
40+
property("Float gteq") = forAll(specFloats, specFloats) { (d1, d2) => numFloat.gteq(d1, d2) == d1 >= d2 }
41+
42+
property("Float equiv") = forAll(specFloats, specFloats) { (d1, d2) => numFloat.equiv(d1, d2) == (d1 == d2) }
43+
44+
property("Float reverse.min") = forAll(specFloats, specFloats) { (d1, d2) => {
45+
val mathmin = math.min(d1, d2)
46+
val numericmin = numFloat.reverse.min(d1, d2)
47+
mathmin == numericmin || mathmin.isNaN && numericmin.isNaN
48+
}
49+
}
50+
51+
property("Float reverse.max") = forAll(specFloats, specFloats) { (d1, d2) => {
52+
val mathmax = math.max(d1, d2)
53+
val numericmax = numFloat.reverse.max(d1, d2)
54+
mathmax == numericmax || mathmax.isNaN && numericmax.isNaN
55+
}
56+
}
57+
58+
property("Float reverse.lt") = forAll(specFloats, specFloats) { (d1, d2) => numFloat.reverse.lt(d1, d2) == d2 < d1 }
59+
60+
property("Float reverse.lteq") = forAll(specFloats, specFloats) { (d1, d2) => numFloat.reverse.lteq(d1, d2) == d2 <= d1 }
61+
62+
property("Float reverse.gt") = forAll(specFloats, specFloats) { (d1, d2) => numFloat.reverse.gt(d1, d2) == d2 > d1 }
63+
64+
property("Float reverse.gteq") = forAll(specFloats, specFloats) { (d1, d2) => numFloat.reverse.gteq(d1, d2) == d2 >= d1 }
65+
66+
property("Float reverse.equiv") = forAll(specFloats, specFloats) { (d1, d2) => numFloat.reverse.equiv(d1, d2) == (d1 == d2) }
67+
68+
69+
val specDoubles: Gen[Double] = oneOf(
70+
Double.MaxValue,
71+
Double.MinPositiveValue,
72+
Double.MinValue,
73+
Double.NaN,
74+
Double.NegativeInfinity,
75+
Double.PositiveInfinity,
76+
-0.0,
77+
+0.0
78+
)
79+
80+
// ticket #5104
81+
property("Double min") = forAll(specDoubles, specDoubles) { (d1, d2) => {
82+
val mathmin = math.min(d1, d2)
83+
val numericmin = d1 min d2
84+
mathmin == numericmin || mathmin.isNaN && numericmin.isNaN
85+
}
86+
}
87+
88+
property("Double max") = forAll(specDoubles, specDoubles) { (d1, d2) => {
89+
val mathmax = math.max(d1, d2)
90+
val numericmax = d1 max d2
91+
mathmax == numericmax || mathmax.isNaN && numericmax.isNaN
92+
}
93+
}
94+
95+
val numDouble = implicitly[Numeric[Double]]
96+
97+
property("Double lt") = forAll(specDoubles, specDoubles) { (d1, d2) => numDouble.lt(d1, d2) == d1 < d2 }
98+
99+
property("Double lteq") = forAll(specDoubles, specDoubles) { (d1, d2) => numDouble.lteq(d1, d2) == d1 <= d2 }
100+
101+
property("Double gt") = forAll(specDoubles, specDoubles) { (d1, d2) => numDouble.gt(d1, d2) == d1 > d2 }
102+
103+
property("Double gteq") = forAll(specDoubles, specDoubles) { (d1, d2) => numDouble.gteq(d1, d2) == d1 >= d2 }
104+
105+
property("Double equiv") = forAll(specDoubles, specDoubles) { (d1, d2) => numDouble.equiv(d1, d2) == (d1 == d2) }
106+
107+
property("Double reverse.min") = forAll(specDoubles, specDoubles) { (d1, d2) => {
108+
val mathmin = math.min(d1, d2)
109+
val numericmin = numDouble.reverse.min(d1, d2)
110+
mathmin == numericmin || mathmin.isNaN && numericmin.isNaN
111+
}
112+
}
113+
114+
property("Double reverse.max") = forAll(specDoubles, specDoubles) { (d1, d2) => {
115+
val mathmax = math.max(d1, d2)
116+
val numericmax = numDouble.reverse.max(d1, d2)
117+
mathmax == numericmax || mathmax.isNaN && numericmax.isNaN
118+
}
119+
}
120+
121+
property("Double reverse.lt") = forAll(specDoubles, specDoubles) { (d1, d2) => numDouble.reverse.lt(d1, d2) == d2 < d1 }
122+
123+
property("Double reverse.lteq") = forAll(specDoubles, specDoubles) { (d1, d2) => numDouble.reverse.lteq(d1, d2) == d2 <= d1 }
124+
125+
property("Double reverse.gt") = forAll(specDoubles, specDoubles) { (d1, d2) => numDouble.reverse.gt(d1, d2) == d2 > d1 }
126+
127+
property("Double reverse.gteq") = forAll(specDoubles, specDoubles) { (d1, d2) => numDouble.reverse.gteq(d1, d2) == d2 >= d1 }
128+
129+
property("Double reverse.equiv") = forAll(specDoubles, specDoubles) { (d1, d2) => numDouble.reverse.equiv(d1, d2) == (d1 == d2) }
130+
}

0 commit comments

Comments
 (0)