Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions kernel/src/main/scala/cats/kernel/instances/DoubleInstances.scala
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,23 @@ class DoubleGroup extends CommutativeGroup[Double] {

class DoubleOrder extends Order[Double] with Hash[Double] {

def hash(x: Double): Int = x.hashCode()
// Canonicalize +0.0 / -0.0 so Hash agrees with eqv (primitive ==).
// java.lang.Double.hashCode distinguishes the two zeros via bit patterns.
def hash(x: Double): Int =
java.lang.Double.hashCode(if (x == 0.0) 0.0 else x)

/**
* Compare using primitive relational operators so +0.0 and -0.0 are equal,
* matching [[eqv]] / IEEE floating equality. `java.lang.Double.compare`
* implements a total order that treats them as different, which made
* `Order[Option[Double]].eqv` disagree with `Order[Double].eqv` (#4807).
* NaN values fall through to `Double.compare` for a stable total order.
*/
def compare(x: Double, y: Double): Int =
java.lang.Double.compare(x, y)
if (x < y) -1
else if (x > y) 1
else if (x == y) 0
else java.lang.Double.compare(x, y)
Comment on lines +52 to +55

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first thing that Double.compare does is this:

public static int compare(double d1, double d2) {
    if (d1 < d2)
        return -1;           // Neither val is NaN, thisVal is smaller
    if (d1 > d2)
        return 1;            // Neither val is NaN, thisVal is larger

    ...

Is there a reason for repeating those < and > comparisons in our code?
How about this:

def compare(x: Double, y: Double): Int =
  if (x == y) 0
  else java.lang.Double.compare(x, y)


override def eqv(x: Double, y: Double): Boolean = x == y
override def neqv(x: Double, y: Double): Boolean = x != y
Expand Down
17 changes: 15 additions & 2 deletions kernel/src/main/scala/cats/kernel/instances/FloatInstances.scala
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,23 @@ class FloatGroup extends CommutativeGroup[Float] {
*/
class FloatOrder extends Order[Float] with Hash[Float] {

def hash(x: Float): Int = x.hashCode()
// Canonicalize +0.0f / -0.0f so Hash agrees with eqv (primitive ==).
// java.lang.Float.hashCode distinguishes the two zeros via bit patterns.
def hash(x: Float): Int =
java.lang.Float.hashCode(if (x == 0.0f) 0.0f else x)

/**
* Compare using primitive relational operators so +0.0f and -0.0f are equal,
* matching [[eqv]] / IEEE floating equality. `java.lang.Float.compare`
* implements a total order that treats them as different, which made
* `Order[Option[Float]].eqv` disagree with `Order[Float].eqv` (#4807).
* NaN values fall through to `Float.compare` for a stable total order.
*/
def compare(x: Float, y: Float): Int =
java.lang.Float.compare(x, y)
if (x < y) -1
else if (x > y) 1
else if (x == y) 0
else java.lang.Float.compare(x, y)

override def eqv(x: Float, y: Float): Boolean = x == y
override def neqv(x: Float, y: Float): Boolean = x != y
Expand Down
25 changes: 25 additions & 0 deletions tests/shared/src/test/scala/cats/tests/OrderSuite.scala
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,31 @@ class OrderSuite extends CatsSuite {
assert(OrderOfCmpSub.gt(OrderSuite.CmpSub(2, "ignored"), OrderSuite.CmpSub(1, "ignored")))
assert(OrderOfCmpSub.eqv(OrderSuite.CmpSub(1, "a"), OrderSuite.CmpSub(1, "b")))
}

// #4807: +0.0 and -0.0 must agree across eqv, compare, and derived Option eqv
test("#4807 Double signed zeros are equal under Order and Option") {

@satorg satorg Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests are pretty comprehensive, thank you!
However, I'd suggest several improvements, because:

  1. Tests for Doube and Float are identical.
  2. The tests always have 0.0 as LHS and -0.0 as RHS. However, we probably should be checking -0.0 vs 0.0 too in order to prevent asymmetry.

That suggests that the checks could probably be extracted into a generic function and then run for

  • (0.0, -0.0)
  • (-0.0, 0.0)
  • (0.0f, -0.0f)
  • (-0.0f, 0.0f)

Also, it probably makes sense to split the tests according the type classes being tested:

  • Order checks should stay in OrderSuite
  • Hash checks in HashSuite
  • Eq checks in EqSuite.

I realize that this specific PR is focused on the specific issue, but long-term it may be confusing to see tests for Hash or Eq here in OrderSuite.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I’m not sure it’s generally a good idea to include the issue number in the test name. Test names should clearly describe what is being tested, rather than the issue that prompted the PR. If cross-referencing is useful (which it probably is) – adding the issue number to a code comment should be sufficient.

val pos = 0.0
val neg = -0.0
assert(Order[Double].eqv(pos, neg))
assertEquals(Order[Double].compare(pos, neg), 0)
assert(cats.kernel.Hash[Double].hash(pos) === cats.kernel.Hash[Double].hash(neg))
assert(Order[Option[Double]].eqv(Some(pos), Some(neg)))
assertEquals(Order[Option[Double]].compare(Some(pos), Some(neg)), 0)
assert((Some(pos): Option[Double]) === (Some(neg): Option[Double]))
assert(pos === neg)
}

test("#4807 Float signed zeros are equal under Order and Option") {
val pos = 0.0f
val neg = -0.0f
assert(Order[Float].eqv(pos, neg))
assertEquals(Order[Float].compare(pos, neg), 0)
assert(cats.kernel.Hash[Float].hash(pos) === cats.kernel.Hash[Float].hash(neg))
assert(Order[Option[Float]].eqv(Some(pos), Some(neg)))
assertEquals(Order[Option[Float]].compare(Some(pos), Some(neg)), 0)
assert((Some(pos): Option[Float]) === (Some(neg): Option[Float]))
assert(pos === neg)
}
}

object OrderSuite {
Expand Down