ionspin / kotlin-multiplatform-bignum

A Kotlin multiplatform library for arbitrary precision arithmetics
Apache License 2.0
339 stars 40 forks source link

add/subtract loose the scale when value is zero #284

Open glureau opened 3 months ago

glureau commented 3 months ago

Describe the bug

Operations like addition or subtraction are loosing the scale when one of the operand value is zero with a scale of zero.

To Reproduce

println((BigDecimal.parseString("1").scale(0) + BigDecimal.parseString("2").scale(1)).scale) // 1
println((BigDecimal.parseString("0").scale(1) + BigDecimal.parseString("2").scale(1)).scale) // 1
println((BigDecimal.parseString("0").scale(0) + BigDecimal.parseString("2").scale(1)).scale) // -1 : KO, expected 1
println((BigDecimal.parseString("1").scale(0) - BigDecimal.parseString("2").scale(1)).scale) // 1
println((BigDecimal.parseString("0").scale(0) - BigDecimal.parseString("2").scale(1)).scale) // -1 : KO, expected 1

Expected behavior A clear and concise description of what you expected to happen.

Platform

Additional context I read a bit the code, I presume it's related to this initial condition, as decimalMode.isPrecisionUnlimited is true (because I don't really need to define the precision here) then I don't calculate the best scale. https://github.com/ionspin/kotlin-multiplatform-bignum/blob/1f61d2a172237c7ee3aa7771a016749ae750481b/bignum/src/commonMain/kotlin/com/ionspin/kotlin/bignum/decimal/BigDecimal.kt#L1561-L1580

I'm open to push a PR if there's a clear way to go. I guess I could change this DEFAULT with a .copy(scale = ...) but no idea if it's the right approach.

PS : Thanks for your work, greatly appreciated!

ionspin commented 3 months ago

Hi @glureau, thanks for reporting and for offering help, it's appreciated! I'll give the issue a look over the weekend and get back to you.

ionspin commented 3 months ago

Sorry didn't have time this weekend to look into it, hopefully next one :(

ionspin commented 3 months ago

Hi @glureau, I got some time to look into this, it seems that the original issue is that when scale was applied to zero, it would incorrectly have a decimalPrecision set to 0 instead of 1, and that signaled isPrecisionUnlimited as true, which I think is not correct.

If you take a look at what happens when scale is applied, in last line of this block we are creating a new BigDecimal

https://github.com/ionspin/kotlin-multiplatform-bignum/blob/1f61d2a172237c7ee3aa7771a016749ae750481b/bignum/src/commonMain/kotlin/com/ionspin/kotlin/bignum/decimal/BigDecimal.kt#L1053-L1066

And then if we follow what happens next we can see that we end up in line 88 that sets the precision to 0, and that I think is not correct.

https://github.com/ionspin/kotlin-multiplatform-bignum/blob/1f61d2a172237c7ee3aa7771a016749ae750481b/bignum/src/commonMain/kotlin/com/ionspin/kotlin/bignum/decimal/BigDecimal.kt#L76-L97

I think the fix should go there, and hardcode the precision in line 88 (when wrk is determined to be equal to zero) to a value of 1, because I think precision of zero is irrelevant as long as it's not 0.

Also I think that multiplication to calculate the exponent is pointless since the significand is zero, but I can't recall the reason why I wrote it like that, maybe it was a copy-paste from some case where it had meaning.

Would you like to look into this and submit a PR?