pjfanning commented on PR #96:
URL: https://github.com/apache/poi-xmlbeans/pull/96#issuecomment-5431767105
Thanks for the patch and for the clear write-up - the diagnosis is right,
and the operand swap is a pure short-circuit reordering with no semantic change
(under `scale < 0` the value is integral, so `toLong(v) == 0` and `signum() ==
0` agree whenever `toLong` doesn't throw). It does fix the reported case.
However, I think it's an incomplete fix: the `scale < 0` guard doesn't stop
`toLong()` from throwing, it only narrows which values reach it. Negative-scale
decimals wider than a long still fail the same way after the patch:
```
1E+20 scale=-20 -> IllegalArgumentException: Value can't be converted
to long
-1E+20 scale=-20 -> IllegalArgumentException: Value can't be converted
to long
1E+2000 scale=-2000 -> IllegalArgumentException: BigDecimal magnitude too
large to convert safely (limit 1024)
```
That shape isn't hypothetical - `new
BigDecimal("100000000000000000000").stripTrailingZeros()` is exactly `1E+20`,
and `BigDecimal.valueOf(unscaled, negativeScale)` gives the same. Values set
programmatically via `setBigDecimalValue` reach `printDecimal` that way (the
lexer path can't, since `lexDecimal` rejects exponents).
`MathUtil.toLong(value) == 0` is really standing in for "is this value zero"
- Harmony's original line was `if (scale == 0 || (isZero() && scale < 0))`. So
I'd suggest using `signum()` directly instead: same semantics, can't throw,
O(1), and it takes `MathUtil` off the serialization path entirely:
```java
if (scale == 0 || (value.signum() == 0 && scale < 0)) {
return intStr;
}
```
I ran the full `printDecimal` body with that condition:
```
1E+20 -> 100000000000000000000
-1E+20 -> -100000000000000000000
0E+5 -> 0
0E-5 -> 0.00000
100 / 1E+2 -> 100
123456789012345678901234567890.5 -> 123456789012345678901234567890.5 (both
signs)
```
all correct, including the zero-with-negative-scale case the branch exists
for.
On the test: `printDecimalHandlesValuesWiderThanLong` only covers `scale >
0`. Worth adding the negative-scale cases, since those are the ones left broken
today:
```java
assertEquals("100000000000000000000", XsTypeConverter.printDecimal(new
BigDecimal("1E+20")));
assertEquals("-100000000000000000000", XsTypeConverter.printDecimal(new
BigDecimal("-1E+20")));
assertEquals("0", XsTypeConverter.printDecimal(new BigDecimal("0E+5")));
```
Minor: with the `signum()` version the inline comment can be simplified to
just say the branch only needs a zero check, which is what Harmony did.
--
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]
