Fix a rounding bug in UnsignedLong.doubleValue() and .floatValue().

See the bug report for a detailed analysis. Fixes #5375. Thanks to @harpocrates (Alex Theriault) for the bug report and suggested fix. RELNOTES=Fixed a rounding bug in `UnsignedLong.doubleValue()`. PiperOrigin-RevId: 368332108
google · Apr 14, 2021 · e61cf2e · e61cf2e
1 parent 1054847
commit e61cf2e
Show file tree

Hide file tree

Showing 4 changed files with 70 additions and 26 deletions.
diff --git a/android/guava-tests/test/com/google/common/primitives/UnsignedLongTest.java b/android/guava-tests/test/com/google/common/primitives/UnsignedLongTest.java
@@ -36,13 +36,23 @@ public class UnsignedLongTest extends TestCase {
   static {
     ImmutableSet.Builder<Long> testLongsBuilder = ImmutableSet.builder();
     ImmutableSet.Builder<BigInteger> testBigIntegersBuilder = ImmutableSet.builder();
+
+    // The values here look like 111...11101...010 in binary, where the initial 111...1110 takes
+    // up exactly as many bits as can be represented in the significand (24 for float, 53 for
+    // double). That final 0 should be rounded up to 1 because the remaining bits make that number
+    // slightly nearer.
+    long floatConversionTest = 0xfffffe8000000002L;
+    long doubleConversionTest = 0xfffffffffffff402L;
+
     for (long i = -3; i <= 3; i++) {
       testLongsBuilder
           .add(i)
           .add(Long.MAX_VALUE + i)
           .add(Long.MIN_VALUE + i)
           .add(Integer.MIN_VALUE + i)
-          .add(Integer.MAX_VALUE + i);
+          .add(Integer.MAX_VALUE + i)
+          .add(floatConversionTest + i)
+          .add(doubleConversionTest + i);
       BigInteger bigI = BigInteger.valueOf(i);
       testBigIntegersBuilder
           .add(bigI)
@@ -130,17 +140,26 @@ public void testToStringRadixQuick() {
     }
   }
 
+  @AndroidIncompatible // b/28251030, re-enable when the fix is everywhere we run this test
   public void testFloatValue() {
     for (long value : TEST_LONGS) {
       UnsignedLong unsignedValue = UnsignedLong.fromLongBits(value);
-      assertEquals(unsignedValue.bigIntegerValue().floatValue(), unsignedValue.floatValue());
+      assertEquals(
+          "Float value of " + unsignedValue,
+          unsignedValue.bigIntegerValue().floatValue(),
+          unsignedValue.floatValue(),
+          0.0f);
     }
   }
 
   public void testDoubleValue() {
     for (long value : TEST_LONGS) {
       UnsignedLong unsignedValue = UnsignedLong.fromLongBits(value);
-      assertEquals(unsignedValue.bigIntegerValue().doubleValue(), unsignedValue.doubleValue());
+      assertEquals(
+          "Double value of " + unsignedValue,
+          unsignedValue.bigIntegerValue().doubleValue(),
+          unsignedValue.doubleValue(),
+          0.0);
     }
   }
 

diff --git a/android/guava/src/com/google/common/primitives/UnsignedLong.java b/android/guava/src/com/google/common/primitives/UnsignedLong.java
@@ -195,12 +195,12 @@ public long longValue() {
    */
   @Override
   public float floatValue() {
-    @SuppressWarnings("cast")
-    float fValue = (float) (value & UNSIGNED_MASK);
-    if (value < 0) {
-      fValue += 0x1.0p63f;
+    if (value >= 0) {
+      return (float) value;
     }
-    return fValue;
+    // The top bit is set, which means that the float value is going to come from the top 24 bits.
+    // So we can ignore the bottom 8, except for rounding. See doubleValue() for more.
+    return (float) ((value >>> 1) | (value & 1)) * 2f;
   }
 
   /**
@@ -209,12 +209,15 @@ public float floatValue() {
    */
   @Override
   public double doubleValue() {
-    @SuppressWarnings("cast")
-    double dValue = (double) (value & UNSIGNED_MASK);
-    if (value < 0) {
-      dValue += 0x1.0p63;
+    if (value >= 0) {
+      return (double) value;
     }
-    return dValue;
+    // The top bit is set, which means that the double value is going to come from the top 53 bits.
+    // So we can ignore the bottom 11, except for rounding. We can unsigned-shift right 1, aka
+    // unsigned-divide by 2, and convert that. Then we'll get exactly half of the desired double
+    // value. But in the specific case where the bottom two bits of the original number are 01, we
+    // want to replace that with 1 in the shifted value for correct rounding.
+    return (double) ((value >>> 1) | (value & 1)) * 2.0;
   }
 
   /** Returns the value of this {@code UnsignedLong} as a {@link BigInteger}. */

diff --git a/guava-tests/test/com/google/common/primitives/UnsignedLongTest.java b/guava-tests/test/com/google/common/primitives/UnsignedLongTest.java
@@ -36,13 +36,23 @@ public class UnsignedLongTest extends TestCase {
   static {
     ImmutableSet.Builder<Long> testLongsBuilder = ImmutableSet.builder();
     ImmutableSet.Builder<BigInteger> testBigIntegersBuilder = ImmutableSet.builder();
+
+    // The values here look like 111...11101...010 in binary, where the initial 111...1110 takes
+    // up exactly as many bits as can be represented in the significand (24 for float, 53 for
+    // double). That final 0 should be rounded up to 1 because the remaining bits make that number
+    // slightly nearer.
+    long floatConversionTest = 0xfffffe8000000002L;
+    long doubleConversionTest = 0xfffffffffffff402L;
+
     for (long i = -3; i <= 3; i++) {
       testLongsBuilder
           .add(i)
           .add(Long.MAX_VALUE + i)
           .add(Long.MIN_VALUE + i)
           .add(Integer.MIN_VALUE + i)
-          .add(Integer.MAX_VALUE + i);
+          .add(Integer.MAX_VALUE + i)
+          .add(floatConversionTest + i)
+          .add(doubleConversionTest + i);
       BigInteger bigI = BigInteger.valueOf(i);
       testBigIntegersBuilder
           .add(bigI)
@@ -130,17 +140,26 @@ public void testToStringRadixQuick() {
     }
   }
 
+  @AndroidIncompatible // b/28251030, re-enable when the fix is everywhere we run this test
   public void testFloatValue() {
     for (long value : TEST_LONGS) {
       UnsignedLong unsignedValue = UnsignedLong.fromLongBits(value);
-      assertEquals(unsignedValue.bigIntegerValue().floatValue(), unsignedValue.floatValue());
+      assertEquals(
+          "Float value of " + unsignedValue,
+          unsignedValue.bigIntegerValue().floatValue(),
+          unsignedValue.floatValue(),
+          0.0f);
     }
   }
 
   public void testDoubleValue() {
     for (long value : TEST_LONGS) {
       UnsignedLong unsignedValue = UnsignedLong.fromLongBits(value);
-      assertEquals(unsignedValue.bigIntegerValue().doubleValue(), unsignedValue.doubleValue());
+      assertEquals(
+          "Double value of " + unsignedValue,
+          unsignedValue.bigIntegerValue().doubleValue(),
+          unsignedValue.doubleValue(),
+          0.0);
     }
   }
 

diff --git a/guava/src/com/google/common/primitives/UnsignedLong.java b/guava/src/com/google/common/primitives/UnsignedLong.java
@@ -195,12 +195,12 @@ public long longValue() {
    */
   @Override
   public float floatValue() {
-    @SuppressWarnings("cast")
-    float fValue = (float) (value & UNSIGNED_MASK);
-    if (value < 0) {
-      fValue += 0x1.0p63f;
+    if (value >= 0) {
+      return (float) value;
     }
-    return fValue;
+    // The top bit is set, which means that the float value is going to come from the top 24 bits.
+    // So we can ignore the bottom 8, except for rounding. See doubleValue() for more.
+    return (float) ((value >>> 1) | (value & 1)) * 2f;
   }
 
   /**
@@ -209,12 +209,15 @@ public float floatValue() {
    */
   @Override
   public double doubleValue() {
-    @SuppressWarnings("cast")
-    double dValue = (double) (value & UNSIGNED_MASK);
-    if (value < 0) {
-      dValue += 0x1.0p63;
+    if (value >= 0) {
+      return (double) value;
     }
-    return dValue;
+    // The top bit is set, which means that the double value is going to come from the top 53 bits.
+    // So we can ignore the bottom 11, except for rounding. We can unsigned-shift right 1, aka
+    // unsigned-divide by 2, and convert that. Then we'll get exactly half of the desired double
+    // value. But in the specific case where the bottom two bits of the original number are 01, we
+    // want to replace that with 1 in the shifted value for correct rounding.
+    return (double) ((value >>> 1) | (value & 1)) * 2.0;
   }
 
   /** Returns the value of this {@code UnsignedLong} as a {@link BigInteger}. */