Re: [PR] Fix printDecimal regression for decimal values wide r than a long [poi-xmlbeans]

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]