From ab785481100a56aff1ebc81a6df61373e2653d14 Mon Sep 17 00:00:00 2001 From: Rikito Taniguchi Date: Mon, 3 Nov 2025 17:17:09 +0900 Subject: [PATCH] Fix too restrictive optimization in Java RyuFloat impl. When `e2 < 0`, current `RyuFloat` calculates `dvIsTrailingZeros` by the code below, which should be equivalent to `mv%2^(q-1) == 0`. ```java dvIsTrailingZeros = (q < FLOAT_MANTISSA_BITS) && (mv & ((1 << (q - 1)) - 1)) == 0; ``` https://github.com/ulfjack/ryu/blob/1264a946ba66eab320e927bfd2362e0c8580c42f/src/main/java/info/adams/ryu/RyuFloat.java#L207 If I understand correctly, `(q < FLOAT_MANTISSA_BITS)` is an optimization that works as follows: - The maximum value of `v` is `2^FLOAT_MANTISSA_BITS - 1` - Therefore, when `q` is greater than or equal to `FLOAT_MANTISSA_BITS`, `v` is never divisible by `2^q` - So, if `(q < FLOAT_MANTISSA_BITS)` is false, we can skip the `(mv & ((1 << (q - 1)) - 1)) == 0` check **However, I think it should be `(q - 1 < FLOAT_MANTISSA_BITS + 2)` because**: - We're checking if `mv` is divisible by `2^(q-1)`, not `2^q`. So the left-hand side should be `q - 1` - The maximum value of `mv` is larger than `2^FLOAT_MANTISSA_BITS - 1`: - `mv` is `4 * m`, where `m` is the 23-bit mantissa. This means `mv <= 4 * (2^23 - 1) = 2^25 - 4` - To check if `mv` is divisible by `2^(q-1)`, the condition `q < 23` is too restrictive, because `mv` can be divisible by up to `2^24` - Therefore, the right-hand side should be `FLOAT_MANTISSA_BITS + 2` --- Actually, a test from `f2s_test.cc`'s `lotsOfTrailingZeros` fails in the Java implementation: https://github.com/ulfjack/ryu/blob/1264a946ba66eab320e927bfd2362e0c8580c42f/ryu/tests/f2s_test.cc#L69 ``` 1) lotsOfTrailingZeros(info.adams.ryu.RyuFloatTest) org.junit.ComparisonFailure: expected:<0.002441406[2]> but was:<0.002441406[3]> ``` This is because: - `mv = 41943040`, `e2 = -34`, and `q = 23` - Therefore, `dvIsTrailingZeros = false` because `q < 23` is false (but it should be true) - By step 4, `dv` is reduced from `244140625` to `24414062` (with `lastRemovedDigit = 5`) - The condition `(dvIsTrailingZeros && (lastRemovedDigit == 5) && (dv % 2 == 0))` evaluates to false (but it should be true!), because `dvIsTrailingZeros = false` https://github.com/ulfjack/ryu/blob/1264a946ba66eab320e927bfd2362e0c8580c42f/src/main/java/info/adams/ryu/RyuFloat.java#L267-L269 As a result, it is rounded up to `24414063`, but it should be rounded to `24414062`. --- src/main/java/info/adams/ryu/RyuFloat.java | 2 +- src/test/java/info/adams/ryu/FloatToStringTest.java | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/main/java/info/adams/ryu/RyuFloat.java b/src/main/java/info/adams/ryu/RyuFloat.java index c4b05cc9..43d134d7 100644 --- a/src/main/java/info/adams/ryu/RyuFloat.java +++ b/src/main/java/info/adams/ryu/RyuFloat.java @@ -204,7 +204,7 @@ public static String floatToString(float value, RoundingMode roundingMode) { } dpIsTrailingZeros = 1 >= q; - dvIsTrailingZeros = (q < FLOAT_MANTISSA_BITS) && (mv & ((1 << (q - 1)) - 1)) == 0; + dvIsTrailingZeros = (q < FLOAT_MANTISSA_BITS + 3) && (mv & ((1 << (q - 1)) - 1)) == 0; dmIsTrailingZeros = (mm % 2 == 1 ? 0 : 1) >= q; } if (DEBUG) { diff --git a/src/test/java/info/adams/ryu/FloatToStringTest.java b/src/test/java/info/adams/ryu/FloatToStringTest.java index 3925f72a..3ebc655a 100644 --- a/src/test/java/info/adams/ryu/FloatToStringTest.java +++ b/src/test/java/info/adams/ryu/FloatToStringTest.java @@ -76,6 +76,14 @@ public void roundingModeEven() { assertF2sEquals("3.436672E10", "3.4366718E10", 3.4366717E10f); } + @Test + public void lotsOfTrailingZeros() { + assertF2sEquals("2.4414062E-4", 2.4414062E-4f); + assertF2sEquals("0.0024414062", 2.4414062E-3f); + assertF2sEquals("0.0043945312", 4.3945312E-3f); + assertF2sEquals("0.0063476562", 6.3476562E-3f); + } + @Test public void roundingEvenIfTied() { assertF2sEquals("0.33007812", 0.33007812f);