SR-7650: Incorrect result adding and subtracting Decimals #1550
Conversation
- For integerAddition, propogate the carry if set. - For integerSubtraction, compute the borrow and set .underflow on error. - When initialising a Decimal() from an Int or UInt dont convert via a double.
|
@swift-ci please test |
|
@phausler @stephentyrone Have you had a chance to review this fix? |
|
@spevans Sorry, I have not. I should be able to review it sometime this week, please ping me if I haven't done so by Friday. |
|
Hi all, I fully respect that we all have jobs to do, but I'm wondering if anybody has realised how serious this is? There is production code out there using Decimal and NSDecimalNumber for currency calculations and a core Swift framework is giving the wrong result for basic arithmetic. This could have serious impact on the reputation of Swift and the frameworks that are based on it. I would have thought a bug like this justified immediate release of a patch version of the foundation library and hopefully a patch release of the Swift tools so that production systems can start using it immediately. Again, no disrespect but perhaps somebody could educate me as to the path and timeframe that will be followed to get this out to production? |
|
@dmonagle That makes it more important to give it a proper review, which I personally will not have time to do until later this week. I'm sorry, but I will not simply rubber stamp a change because it's important. I understand that you want your change to go in as quickly as possible; I'm frequently frustrated waiting on reviews myself, but I promise that I will get to it as soon as my other projects allow me to do so, and I expect that @phausler will do the same. @parkera is the person to address any schedule questions. |
|
@stephentyrone please understand this is not my pull request. I am also not asking you to "rubber stamp" something without review. However I do think this problem rates as being more than merely "important", which is why I felt the need to comment. In the meantime I think it's only right to make sure that this gets some circulation as anybody using Decimal in production for arithmetic should immediately start looking for a workaround. |
|
@dmonagle Thanks for raising the importance of this change. We'll get it reviewed as soon as possible and then merged into the right branches. |
Foundation/Decimal.swift
Outdated
| let c:UInt32 = min(left._length, right._length) | ||
| var i: UInt32 = 0 | ||
| var carry: UInt16 = 0 | ||
| let c: UInt32 = min(left._length, right._length) |
There was a problem hiding this comment.
I'm not blocking the patch on this, but it'd be nice to have more descriptive names here.
Foundation/Decimal.swift
Outdated
|
|
||
| while i < c { | ||
| let li = UInt32(left[i]) | ||
| let ri = UInt32(right[i]) | ||
| accumulator = li + ri + UInt32(carry) | ||
| let accumulator = li + ri + UInt32(carry) |
There was a problem hiding this comment.
Wait, why is something that's called an accumulator destroyed every cycle? I understand you're using it here to mean the 32-bit value that's about to go in the i-th word, but then it isn't an accumulator any longer. (Why was it being accumulated in the original? Why is it no longer used to accumulate? This code sorely needs commenting.)
To be clear: the word-by-word approach makes sense to me, and the old one may just be incorrect, but if we are to make it substantially better we should make sure a casual reader can handle this code down the line.
There was a problem hiding this comment.
(I have since writing this verified on paper that it looks correct to me.)
| XCTAssertEqual(Decimal(0x1_0000_0000_0000) - Decimal(0x1000), Decimal(0xFFFFFFFFF000)) | ||
| XCTAssertEqual(Decimal(1234_5678_9012_3456_7899 as UInt64) - Decimal(1234_5678_9012_3456_7890 as UInt64), Decimal(9)) | ||
| XCTAssertEqual(Decimal(0xffdd_bb00_8866_4422 as UInt64) - Decimal(0x7777_7777), Decimal(0xFFDD_BB00_10EE_CCAB as UInt64)) | ||
| XCTAssertEqual(NSDecimalNumber(floatLiteral: 5538).subtracting(NSDecimalNumber(floatLiteral: 2880.4)), NSDecimalNumber(floatLiteral: 5538 - 2880.4)) |
There was a problem hiding this comment.
I’d rather these be initialized from strings to avoid floating-point representation shenanigans.
There was a problem hiding this comment.
Where ‘these’ are all Decimal/NSDecimalNumber instances created from floating-point values.
- integerAdd(), integerSubtract(): Use more meaningful variable names.
|
@swift-ci please test |
|
with the renamed variables this is much clearer and looks quite reasonable to me. The tests added pass on Darwin so this definitely brings things more into line with the two platforms. Sorry for the delay on the review. |
|
@swift-ci please test |
| let li = UInt32(left[idx]) | ||
| let ri = UInt32(right[idx]) | ||
| // 0x10000 is to borrow in advance to avoid underflow. | ||
| let difference: UInt32 = (0x10000 + li) - UInt32(borrow) - ri |
There was a problem hiding this comment.
This is a really over-complicated way to do this instead of using the wrapping operators, but it's correct, so it's OK for now.
|
@swift-ci please test and merge |
For integerAddition, propogate the carry if set.
For integerSubtraction, compute the borrow and set .underflow
on error.
When initialising a Decimal() from an Int or UInt dont convert via
a double.