Fix spurious overflow in Fraction.multiplyBy for unreduced operands - #1769
Fix spurious overflow in Fraction.multiplyBy for unreduced operands#1769alhudz wants to merge 2 commits into
Conversation
The Knuth 4.5.1 cross-gcd cancels only the cross terms and assumes both operands are reduced, so an unreduced operand can overflow the intermediate int product even when the reduced result fits. Reduce both operands before the multiply; divideBy and pow route through multiplyBy.
There was a problem hiding this comment.
Pull request overview
Fixes Fraction.multiplyBy (and, transitively, divideBy/pow) throwing spurious ArithmeticException overflows when multiplying/dividing unreduced operands whose reduced result still fits in int.
Changes:
- Reduce both operands before the Knuth 4.5.1 cross-GCD multiply to prevent intermediate
intoverflow when the final reduced result is representable. - Add unit tests covering multiply/divide with unreduced operands that previously triggered
mulPosoverflow.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/main/java/org/apache/commons/lang3/math/Fraction.java | Reduces operands before cross-cancellation in multiplyBy to avoid intermediate overflow for unreduced inputs. |
| src/test/java/org/apache/commons/lang3/math/FractionTest.java | Adds regression tests for multiply/divide cases where unreduced operands previously caused spurious overflow. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| final Fraction a = reduce(); | ||
| final Fraction b = fraction.reduce(); | ||
| final int d1 = greatestCommonDivisor(a.numerator, b.denominator); | ||
| final int d2 = greatestCommonDivisor(b.numerator, a.denominator); | ||
| return getReducedFraction(mulAndCheck(a.numerator / d1, b.numerator / d2), mulPosAndCheck(a.denominator / d2, b.denominator / d1)); |
There was a problem hiding this comment.
Done, switched to reducing into local ints instead of calling reduce(), so no intermediate Fraction allocations now. Behaviour is unchanged: the repro getFraction(-1, 46341).multiplyBy(getFraction(100, 1000000)) still returns -1/463410000, and FractionTest plus checkstyle:check pass.
|
@alhudz Please review Copilot comments. |
|
Hello @alhudz Thank you! |
Repro:Fraction.getFraction(-1, 46341).multiplyBy(Fraction.getFraction(100, 1000000))throwsArithmeticException: overflow: mulPos, yet the same value with a reduced operand,Fraction.getFraction(-1, 46341).multiplyBy(Fraction.getFraction(1, 10000)), returns-1/463410000.divideByandpowroute throughmultiplyByand behave the same way.Cause: the Knuth 4.5.1 cross-gcd (d1 = gcd(numerator, fraction.denominator),d2 = gcd(fraction.numerator, denominator)) cancels the cross terms only and assumes both operands are already in lowest terms. AFractionfromgetFractionis not reduced, so a factor shared inside an operand survives into themulAndCheck/mulPosAndCheckproduct and overflows anintbeforegetReducedFractioncan cancel it, although the resulting numerator (-1) and denominator (463410000) both fit. The Javadoc only permits a throw when the resulting numerator or denominator exceedsInteger.MAX_VALUE.Fix: reduce both operands before the cross-gcd multiply. The product still passes throughgetReducedFraction, so the value is unchanged and the operation now overflows only when the reduced result genuinely exceedsint.add/subtractare untouched; they return unreduced results by design.mvn; that'smvnon the command line by itself.