pjfanning commented on code in PR #1473:
URL: https://github.com/apache/poi/pull/1473#discussion_r4368635503
##########
poi/src/main/java/org/apache/poi/ss/format/CellNumberFormatter.java:
##########
@@ -680,7 +682,13 @@ private void writeFraction(double value, StringBuffer
result,
} else {
// buildFractionMaxDenominator only gives denominators below the
maximum it is passed, so pass
// 10^digits, as FractionFormat does: e.g. "??/??" shows 0.99 as
98/99, where 99 would give 1/1
- SimpleFraction frac =
SimpleFraction.buildFractionMaxDenominator(fractional, maxDenominator + 1);
+ SimpleFraction frac;
+ try {
+ frac = SimpleFraction.buildFractionMaxDenominator(fractional,
maxDenominator + 1);
+ } catch (RuntimeException e) {
+ LOG.atError().withThrowable(e).log("error while fraction
evaluation");
+ frac = new
SimpleFraction(Math.toIntExact(Math.round(fractional)), 1);
Review Comment:
This fallback can throw too. When `buildFractionMaxDenominator` throws
`IllegalArgumentException` because the value is above `Integer.MAX_VALUE`,
`Math.toIntExact(Math.round(fractional))` overflows for the same reason, so the
error is logged and an `ArithmeticException` escapes anyway:
```java
CellFormat.getInstance("?/?").apply(3e9 + 0.5); // ArithmeticException:
integer overflow
CellFormat.getInstance("??/??").apply(3e9 + 0.5); // same
```
(`"# ?/?"` is fine, since the integer part is split off first.) Trunk throws
here as well, so this isn't a regression, but the pre-#1406 code wrapped the
whole body of `writeFraction`, so it didn't escape there. The fallback
shouldn't be able to throw: either avoid the int conversion for the
whole-number case, or wrap more of the method as before.
##########
poi/src/main/java/org/apache/poi/ss/usermodel/DataFormatter.java:
##########
@@ -555,7 +555,10 @@ protected Format createFormat(double cellValue, int
formatIndex, String sFormat)
if (plainFraction || fractionWithText) {
if (fractionWithText) {
try {
- return new
CellFormatWrapper(CellFormat.getInstance(locale, formatStr));
+ CellFormat cellFormat = CellFormat.getInstance(locale,
formatStr);
+ // a format CellFormat cannot make sense of fails here,
not when formatting
+ cellFormat.apply(cellValue);
Review Comment:
This probe only proves the format works for the first value. `createFormat`
results are cached per format string (`formats.get`/`formats.put` in
`getFormat`), so a later cell with the same format but a value that fails (e.g.
a large improper fraction, see the `CellNumberFormatter` comment) still throws
from `CellFormatWrapper.format`. The comment "fails here, not when formatting"
promises more than this delivers.
The same pattern already exists further down for
`hasLiteralDecimalFormatCannotShow`, so it has the same limitation. With the
9-digit cap the probe is no longer needed for the case this PR fixes. A
try/catch with a fallback inside `CellFormatWrapper.format` would cover every
value. Otherwise, I'd reword the comment to say it only checks the first value.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
