alhudz commented on code in PR #1769:
URL: https://github.com/apache/commons-lang/pull/1769#discussion_r3743435542
##########
src/main/java/org/apache/commons/lang3/math/Fraction.java:
##########
@@ -766,9 +766,14 @@ public Fraction multiplyBy(final Fraction fraction) {
}
// knuth 4.5.1
// make sure we don't overflow unless the result *must* overflow.
- final int d1 = greatestCommonDivisor(numerator, fraction.denominator);
- final int d2 = greatestCommonDivisor(fraction.numerator, denominator);
- return getReducedFraction(mulAndCheck(numerator / d1,
fraction.numerator / d2), mulPosAndCheck(denominator / d2, fraction.denominator
/ d1));
+ // Reduce both operands first: the cross-gcd below cancels the cross
terms only, so a
+ // factor shared inside an unreduced operand survives into the product
and can overflow
+ // an int even when the reduced result fits.
+ 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));
Review Comment:
Done, switched to reducing into local `int`s 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
