This is an automated email from the ASF dual-hosted git repository.
aherbert pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/commons-numbers.git
commit 51973c3294ca0cdfbe711c8549e2609b40d31634
Author: aherbert <aherb...@apache.org>
AuthorDate: Wed Apr 8 13:14:37 2020 +0100
Fix fraction hashCode
The sign must be incorporated into the hash as a fraction with the same
numerical value can have different representations:
-1/3 == 1/-3
Using the sign ensures that -1/3 and 1/3 have different hash codes. This
would not be true if the hash were constructed using only the absolute
values of numerator and denominator.
---
.../org/apache/commons/numbers/fraction/BigFraction.java | 12 +++++++++++-
.../java/org/apache/commons/numbers/fraction/Fraction.java | 12 +++++++++++-
.../org/apache/commons/numbers/fraction/BigFractionTest.java | 10 +++++++---
.../org/apache/commons/numbers/fraction/FractionTest.java | 10 +++++++---
4 files changed, 36 insertions(+), 8 deletions(-)
diff --git
a/commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/BigFraction.java
b/commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/BigFraction.java
index cee2a6e..3962e2e 100644
---
a/commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/BigFraction.java
+++
b/commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/BigFraction.java
@@ -1019,7 +1019,17 @@ public final class BigFraction
@Override
public int hashCode() {
- return 37 * (37 * 17 + numerator.hashCode()) + denominator.hashCode();
+ // Incorporate the sign and absolute values of the numerator and
denominator.
+ // Equivalent to
+ // Arrays.hashCode(new int[] {signum(), numerator.abs(),
denominator.abs()})
+ // int hash = 1;
+ // hash = 31 * hash + signum();
+ // hash = 31 * hash + numerator.abs().hashCode();
+ // hash = 31 * hash + denominator.abs().hashCode();
+ // Note: BigInteger.hashCode() * BigInteger.signum() ==
BigInteger.abs().hashCode().
+ final int numS = numerator.signum();
+ final int denS = denominator.signum();
+ return 31 * (31 * (31 + numS * denS) + numerator.hashCode() * numS) +
denominator.hashCode() * denS;
}
/**
diff --git
a/commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/Fraction.java
b/commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/Fraction.java
index 31e7cde..be85fb3 100644
---
a/commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/Fraction.java
+++
b/commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/Fraction.java
@@ -706,7 +706,17 @@ public final class Fraction
@Override
public int hashCode() {
- return 37 * (37 * 17 + numerator) + denominator;
+ // Incorporate the sign and absolute values of the numerator and
denominator.
+ // Equivalent to
+ // Arrays.hashCode(new int[] {signum(), Math.abs(numerator),
Math.abs(denominator)})
+ // int hash = 1;
+ // hash = 31 * hash + signum();
+ // hash = 31 * hash + Math.abs(numerator);
+ // hash = 31 * hash + Math.abs(denominator);
+ // Note: x * Integer.signum(x) is equivalent to Math.abs(x).
+ final int numS = Integer.signum(numerator);
+ final int denS = Integer.signum(denominator);
+ return 31 * (31 * (31 + numS * denS) + numerator * numS) + denominator
* denS;
}
/**
diff --git
a/commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/BigFractionTest.java
b/commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/BigFractionTest.java
index e3a611b..4d3377c 100644
---
a/commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/BigFractionTest.java
+++
b/commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/BigFractionTest.java
@@ -19,13 +19,12 @@ package org.apache.commons.numbers.fraction;
import java.math.BigDecimal;
import java.math.BigInteger;
import java.math.RoundingMode;
+import java.util.Arrays;
import org.apache.commons.numbers.core.TestUtils;
import org.junit.jupiter.api.Assertions;
-import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
-
public class BigFractionTest {
private static void assertFraction(long expectedNumerator, long
expectedDenominator, BigFraction actual) {
@@ -595,7 +594,6 @@ public class BigFractionTest {
Assertions.assertEquals(new BigDecimal("0.333"), BigFraction.of(1,
3).bigDecimalValue(3, RoundingMode.DOWN));
}
- @Disabled
@Test
public void testEqualsAndHashCode() {
BigFraction zero = BigFraction.of(0, 1);
@@ -649,6 +647,12 @@ public class BigFractionTest {
Assertions.assertNotSame(f1, f2, "Do not call this assertion with the
same object");
Assertions.assertEquals(f1, f2);
Assertions.assertEquals(f1.hashCode(), f2.hashCode(), "Equal fractions
have different hashCode");
+ // Check the hashcode computation.
+ // This is not mandated but is a recommendation.
+ final int expected = Arrays.hashCode(new Object[] {f1.signum(),
+
f1.getNumerator().abs(),
+
f1.getDenominator().abs()});
+ Assertions.assertEquals(expected, f1.hashCode(), "Hashcode not equal
to using Arrays.hashCode");
}
@Test
diff --git
a/commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/FractionTest.java
b/commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/FractionTest.java
index 0888a75..ada4269 100644
---
a/commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/FractionTest.java
+++
b/commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/FractionTest.java
@@ -16,13 +16,12 @@
*/
package org.apache.commons.numbers.fraction;
+import java.util.Arrays;
import org.apache.commons.numbers.core.TestUtils;
import org.junit.jupiter.api.Assertions;
-import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
-
/**
*/
public class FractionTest {
@@ -427,7 +426,6 @@ public class FractionTest {
);
}
- @Disabled
@Test
public void testEqualsAndHashCode() {
Fraction zero = Fraction.of(0, 1);
@@ -492,6 +490,12 @@ public class FractionTest {
Assertions.assertNotSame(f1, f2, "Do not call this assertion with the
same object");
Assertions.assertEquals(f1, f2);
Assertions.assertEquals(f1.hashCode(), f2.hashCode(), "Equal fractions
have different hashCode");
+ // Check the hashcode computation.
+ // This is not mandated but is a recommendation.
+ final int expected = Arrays.hashCode(new int[] {f1.signum(),
+
Math.abs(f1.getNumerator()),
+
Math.abs(f1.getDenominator())});
+ Assertions.assertEquals(expected, f1.hashCode(), "Hashcode not equal
to using Arrays.hashCode");
}
@Test
