Skip to content

Commit c8393fd

Browse files
committed
SI-9087 Fix min/max of reversed Double/Float orderings
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}`.
1 parent 3d76836 commit c8393fd

File tree

3 files changed

+74
-8
lines changed

3 files changed

+74
-8
lines changed

src/library/scala/math/Ordering.scala

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -284,6 +284,9 @@ object Ordering extends LowPriorityOrderingImplicits {
284284
override def gteq(x: Float, y: Float): Boolean = outer.gteq(y, x)
285285
override def lt(x: Float, y: Float): Boolean = outer.lt(y, x)
286286
override def gt(x: Float, y: Float): Boolean = outer.gt(y, x)
287+
override def min(x: Float, y: Float): Float = outer.max(x, y)
288+
override def max(x: Float, y: Float): Float = outer.min(x, y)
289+
287290
}
288291
}
289292
implicit object Float extends FloatOrdering
@@ -309,6 +312,8 @@ object Ordering extends LowPriorityOrderingImplicits {
309312
override def gteq(x: Double, y: Double): Boolean = outer.gteq(y, x)
310313
override def lt(x: Double, y: Double): Boolean = outer.lt(y, x)
311314
override def gt(x: Double, y: Double): Boolean = outer.gt(y, x)
315+
override def min(x: Double, y: Double): Double = outer.max(x, y)
316+
override def max(x: Double, y: Double): Double = outer.min(x, y)
312317
}
313318
}
314319
implicit object Double extends DoubleOrdering

test/files/scalacheck/nan-ordering.scala

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -42,16 +42,16 @@ object Test extends Properties("NaN-Ordering") {
4242
property("Float equiv") = forAll(specFloats, specFloats) { (d1, d2) => numFloat.equiv(d1, d2) == (d1 == d2) }
4343

4444
property("Float reverse.min") = forAll(specFloats, specFloats) { (d1, d2) => {
45-
val mathmin = math.min(d1, d2)
45+
val mathmax = math.max(d1, d2)
4646
val numericmin = numFloat.reverse.min(d1, d2)
47-
mathmin == numericmin || mathmin.isNaN && numericmin.isNaN
47+
mathmax == numericmin || mathmax.isNaN && numericmin.isNaN
4848
}
4949
}
5050

5151
property("Float reverse.max") = forAll(specFloats, specFloats) { (d1, d2) => {
52-
val mathmax = math.max(d1, d2)
52+
val mathmin = math.min(d1, d2)
5353
val numericmax = numFloat.reverse.max(d1, d2)
54-
mathmax == numericmax || mathmax.isNaN && numericmax.isNaN
54+
mathmin == numericmax || mathmin.isNaN && numericmax.isNaN
5555
}
5656
}
5757

@@ -105,16 +105,16 @@ object Test extends Properties("NaN-Ordering") {
105105
property("Double equiv") = forAll(specDoubles, specDoubles) { (d1, d2) => numDouble.equiv(d1, d2) == (d1 == d2) }
106106

107107
property("Double reverse.min") = forAll(specDoubles, specDoubles) { (d1, d2) => {
108-
val mathmin = math.min(d1, d2)
108+
val mathmax = math.max(d1, d2)
109109
val numericmin = numDouble.reverse.min(d1, d2)
110-
mathmin == numericmin || mathmin.isNaN && numericmin.isNaN
110+
mathmax == numericmin || mathmax.isNaN && numericmin.isNaN
111111
}
112112
}
113113

114114
property("Double reverse.max") = forAll(specDoubles, specDoubles) { (d1, d2) => {
115-
val mathmax = math.max(d1, d2)
115+
val mathmin = math.min(d1, d2)
116116
val numericmax = numDouble.reverse.max(d1, d2)
117-
mathmax == numericmax || mathmax.isNaN && numericmax.isNaN
117+
mathmin == numericmax || mathmin.isNaN && numericmax.isNaN
118118
}
119119
}
120120

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
package scala.math
2+
3+
import org.junit.Assert._
4+
import org.junit.Test
5+
import org.junit.runner.RunWith
6+
import org.junit.runners.JUnit4
7+
8+
@RunWith(classOf[JUnit4])
9+
class OrderingTest {
10+
11+
/* Test for SI-9077 */
12+
@Test
13+
def testReverseOrdering {
14+
def check[T: Ordering](t1: T, t2: T): Unit = {
15+
val O = Ordering[T]
16+
val R = O.reverse
17+
assertEquals(O.min(t1, t2), R.max(t1, t2))
18+
assertEquals(O.max(t1, t2), R.min(t1, t2))
19+
20+
assertEquals(O.lteq(t1, t2), R.lteq(t2, t1))
21+
assertEquals(O.lt(t1, t2), R.lt(t2, t1))
22+
assertEquals(O.gteq(t1, t2), R.gteq(t2, t1))
23+
assertEquals(O.gt(t1, t2), R.gt(t2, t1))
24+
assertEquals(O.compare(t1, t2), R.compare(t2, t1))
25+
26+
assertEquals(O.equiv(t1, t2), R.equiv(t1, t2))
27+
28+
assertEquals(O.on((x: T) => x).min(t1, t2), R.on((x: T) => x).max(t1, t2))
29+
30+
assertEquals(O.tryCompare(t1, t2), R.tryCompare(t2, t1))
31+
32+
assertEquals(O.mkOrderingOps(t1).<(t2), R.mkOrderingOps(t2).<(t1))
33+
assertEquals(O.mkOrderingOps(t1).<=(t2), R.mkOrderingOps(t2).<=(t1))
34+
assertEquals(O.mkOrderingOps(t1).>(t2), R.mkOrderingOps(t2).>(t1))
35+
assertEquals(O.mkOrderingOps(t1).>=(t2), R.mkOrderingOps(t2).>=(t1))
36+
37+
assertEquals(O.mkOrderingOps(t1).min(t2), R.mkOrderingOps(t1).max(t2))
38+
assertEquals(O.mkOrderingOps(t1).max(t2), R.mkOrderingOps(t1).min(t2))
39+
}
40+
def checkAll[T: Ordering](ts: T*): Unit = {
41+
for (t1 <- ts; t2 <- ts) check(t1, t2)
42+
}
43+
checkAll[Unit](())
44+
checkAll[Boolean](true, false)
45+
checkAll[Byte](Byte.MinValue, -1.toByte, 0.toByte, 1.toByte, Byte.MaxValue)
46+
checkAll[Char](Char.MinValue, -1.toChar, 0.toChar, 1.toChar, Char.MaxValue)
47+
checkAll[Short](Short.MinValue, -1, 0, 1, Short.MaxValue)
48+
checkAll[Int](Int.MinValue, -1, 0, 1, Int.MaxValue)
49+
checkAll[Double](Double.MinValue, -1, -0, 0, 1, Double.MaxValue)
50+
checkAll[Float](Float.MinValue, -1, -0, 0, 1, Float.MaxValue)
51+
52+
checkAll[BigInt](Int.MinValue, -1, 0, 1, Int.MaxValue)
53+
checkAll[BigDecimal](Int.MinValue, -1, -0, 1, Int.MaxValue)
54+
checkAll[String]("", "a", "b", "bb")
55+
checkAll[String]("", "a", "b", "bb")
56+
checkAll[Option[Int]](None, Some(1), Some(2))
57+
checkAll[Iterable[Int]](Nil, List(1), List(1, 2))
58+
checkAll[(Int, Int)]((1, 2), (1, 3), (4, 5))
59+
}
60+
}
61+

0 commit comments

Comments
 (0)