Copilot commented on code in PR #1769:
URL: https://github.com/apache/commons-lang/pull/1769#discussion_r3741330004
##########
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:
`multiplyBy` now calls `reduce()` on both operands, which can allocate up to
two extra `Fraction` instances (plus the result) for the unreduced-input case
this change targets. Since the method only needs reduced
numerators/denominators, consider reducing into local `int` variables instead
to avoid extra allocations and hash computations while keeping the same
overflow-avoidance behavior.
--
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]
