Skip to content

SR-7650: Incorrect result adding and subtracting Decimals #1550

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 24, 2018
Merged

SR-7650: Incorrect result adding and subtracting Decimals #1550

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
599848b
SR-7650: Incorrect result adding and subtracting Decimals
spevans May 11, 2018
5dd39d3
Decimal: Add more tests for addition and subtraction
spevans May 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
172 changes: 98 additions & 74 deletions Foundation/Decimal.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,12 +441,39 @@ extension Decimal {
self.compact()
}
}

public init(_ value: UInt64) {
self.init(Double(value))
self = Decimal()
if value == 0 {
return
}

var compactValue = value
var exponent: Int32 = 0
while compactValue % 10 == 0 {
compactValue = compactValue / 10
exponent = exponent + 1
}
_isCompact = 1
_exponent = exponent

let wordCount = ((UInt64.bitWidth - compactValue.leadingZeroBitCount) + (UInt16.bitWidth - 1)) / UInt16.bitWidth
_length = UInt32(wordCount)
_mantissa.0 = UInt16(truncatingIfNeeded: compactValue >> 0)
_mantissa.1 = UInt16(truncatingIfNeeded: compactValue >> 16)
_mantissa.2 = UInt16(truncatingIfNeeded: compactValue >> 32)
_mantissa.3 = UInt16(truncatingIfNeeded: compactValue >> 48)
}

public init(_ value: Int64) {
self.init(Double(value))
if value < 0 {
self.init(value == Int64.min ? UInt64(Int64.max) + 1 : UInt64(abs(value)))
_isNegative = 1
} else {
self.init(UInt64(value))
}
}

public init(_ value: UInt) {
self.init(UInt64(value))
}
Expand Down Expand Up @@ -1164,62 +1191,60 @@ public func NSDecimalAdd(_ result: UnsafeMutablePointer<Decimal>, _ leftOperand:
}

fileprivate func integerAdd(_ result: inout WideDecimal, _ left: inout Decimal, _ right: inout Decimal) -> NSDecimalNumber.CalculationError {
var i:UInt32 = 0
var carry:UInt16 = 0
var accumulator:UInt32 = 0

let c:UInt32 = min(left._length, right._length)
var idx: UInt32 = 0
var carry: UInt16 = 0
let maxIndex: UInt32 = min(left._length, right._length) // The highest index with bits set in both values

while i < c {
let li = UInt32(left[i])
let ri = UInt32(right[i])
accumulator = li + ri + UInt32(carry)
carry = UInt16(truncatingIfNeeded:accumulator >> 16)
result[i] = UInt16(truncatingIfNeeded:accumulator)
i += 1
while idx < maxIndex {
let li = UInt32(left[idx])
let ri = UInt32(right[idx])
let sum = li + ri + UInt32(carry)
carry = UInt16(truncatingIfNeeded: sum >> 16)
result[idx] = UInt16(truncatingIfNeeded: sum)
idx += 1
}

while i < left._length {
while idx < left._length {
if carry != 0 {
let li = UInt32(left[i])
accumulator = li + UInt32(carry)
carry = UInt16(truncatingIfNeeded:accumulator >> 16)
result[i] = UInt16(truncatingIfNeeded:accumulator)
i += 1
let li = UInt32(left[idx])
let sum = li + UInt32(carry)
carry = UInt16(truncatingIfNeeded: sum >> 16)
result[idx] = UInt16(truncatingIfNeeded: sum)
idx += 1
} else {
while i < left._length {
result[i] = left[i]
i += 1
while idx < left._length {
result[idx] = left[idx]
idx += 1
}
break
}
}
while i < right._length {
while idx < right._length {
if carry != 0 {
let ri = UInt32(right[i])
accumulator = ri + UInt32(carry)
carry = UInt16(truncatingIfNeeded:accumulator >> 16)
result[i] = UInt16(truncatingIfNeeded:accumulator)
i += 1
let ri = UInt32(right[idx])
let sum = ri + UInt32(carry)
carry = UInt16(truncatingIfNeeded: sum >> 16)
result[idx] = UInt16(truncatingIfNeeded: sum)
idx += 1
} else {
while i < right._length {
result[i] = right[i]
i += 1
while idx < right._length {
result[idx] = right[idx]
idx += 1
}
break
}
}
result._length = idx

if carry != 0 {
if result._length < i {
result._length = i
return .overflow
} else {
result[i] = carry
i += 1
}
result[idx] = carry
idx += 1
result._length = idx
}
result._length = i;
if idx > Decimal.maxSize {
return .overflow
}

return .noError;
}

Expand All @@ -1231,49 +1256,48 @@ fileprivate func integerAdd(_ result: inout WideDecimal, _ left: inout Decimal,
// give b-a...
//
fileprivate func integerSubtract(_ result: inout Decimal, _ left: inout Decimal, _ right: inout Decimal) -> NSDecimalNumber.CalculationError {
var i:UInt32 = 0
var carry:UInt16 = 1
var accumulator:UInt32 = 0

let c:UInt32 = min(left._length, right._length)

while i < c {
let li = UInt32(left[i])
let ri = UInt32(right[i])
accumulator = 0xffff + li - ri + UInt32(carry)
carry = UInt16(truncatingIfNeeded:accumulator >> 16)
result[i] = UInt16(truncatingIfNeeded:accumulator)
i += 1
}

while i < left._length {
if carry != 0 {
let li = UInt32(left[i])
accumulator = 0xffff + li // + no carry
carry = UInt16(truncatingIfNeeded:accumulator >> 16)
result[i] = UInt16(truncatingIfNeeded:accumulator)
i += 1
var idx: UInt32 = 0
let maxIndex: UInt32 = min(left._length, right._length) // The highest index with bits set in both values
var borrow: UInt16 = 0

while idx < maxIndex {
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

result[idx] = UInt16(truncatingIfNeeded: difference)
// borrow = 1 if the borrow was used.
borrow = 1 - UInt16(truncatingIfNeeded: difference >> 16)
idx += 1
}

while idx < left._length {
if borrow != 0 {
let li = UInt32(left[idx])
let sum = 0xffff + li // + no carry
borrow = 1 - UInt16(truncatingIfNeeded: sum >> 16)
result[idx] = UInt16(truncatingIfNeeded: sum)
idx += 1
} else {
while i < left._length {
result[i] = left[i]
i += 1
while idx < left._length {
result[idx] = left[idx]
idx += 1
}
break
}
}
while i < right._length {
let ri = UInt32(right[i])
accumulator = 0xffff - ri + UInt32(carry)
carry = UInt16(truncatingIfNeeded:accumulator >> 16)
result[i] = UInt16(truncatingIfNeeded:accumulator)
i += 1
while idx < right._length {
let ri = UInt32(right[idx])
let difference = 0xffff - ri + UInt32(borrow)
borrow = 1 - UInt16(truncatingIfNeeded: difference >> 16)
result[idx] = UInt16(truncatingIfNeeded: difference)
idx += 1
}

if carry != 0 {
if borrow != 0 {
return .overflow
}
result._length = i;

result._length = idx;
result.trimTrailingZeros()

return .noError;
Expand Down
32 changes: 32 additions & 0 deletions TestFoundation/TestDecimal.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,10 @@ class TestDecimal: XCTestCase {
XCTAssertFalse(zero.isInfinite)
XCTAssertFalse(zero.isNaN)
XCTAssertFalse(zero.isSignaling)

let d1 = Decimal(1234567890123456789 as UInt64)
XCTAssertEqual(d1._exponent, 0)
XCTAssertEqual(d1._length, 4)
}
func test_Constants() {
XCTAssertEqual(8, NSDecimalMaxSize)
Expand Down Expand Up @@ -273,7 +277,31 @@ class TestDecimal: XCTestCase {
}
}
}

XCTAssertEqual(Decimal(186243 * 15673 as Int64), Decimal(186243) * Decimal(15673))

XCTAssertEqual(Decimal(string: "5538")! + Decimal(string: "2880.4")!, Decimal(string: "8418.4")!)
XCTAssertEqual(NSDecimalNumber(floatLiteral: 5538).adding(NSDecimalNumber(floatLiteral: 2880.4)), NSDecimalNumber(floatLiteral: 5538 + 2880.4))

XCTAssertEqual(Decimal(string: "5538.0")! - Decimal(string: "2880.4")!, Decimal(string: "2657.6")!)
XCTAssertEqual(Decimal(string: "2880.4")! - Decimal(5538), Decimal(string: "-2657.6")!)
XCTAssertEqual(Decimal(0x10000) - Decimal(0x1000), Decimal(0xf000))
XCTAssertEqual(Decimal(0x1_0000_0000) - Decimal(0x1000), Decimal(0xFFFFF000))
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))
Copy link
Contributor

@millenomi millenomi May 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’d rather these be initialized from strings to avoid floating-point representation shenanigans.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where ‘these’ are all Decimal/NSDecimalNumber instances created from floating-point values.

XCTAssertEqual(NSDecimalNumber(floatLiteral: 2880.4).subtracting(NSDecimalNumber(floatLiteral: 5538)), NSDecimalNumber(floatLiteral: 2880.4 - 5538))

XCTAssertEqual(Decimal.greatestFiniteMagnitude - Decimal.greatestFiniteMagnitude, Decimal(0))
XCTAssertEqual(Decimal.leastFiniteMagnitude - Decimal(1), Decimal.leastFiniteMagnitude)
let overflowed = Decimal.greatestFiniteMagnitude + Decimal.greatestFiniteMagnitude
XCTAssertTrue(overflowed.isNaN)

let highBit = Decimal(_exponent: 0, _length: 8, _isNegative: 0, _isCompact: 1, _reserved: 0, _mantissa: (0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x8000))
let otherBits = Decimal(_exponent: 0, _length: 8, _isNegative: 0, _isCompact: 1, _reserved: 0, _mantissa: (0xffff, 0xffff, 0xffff, 0xffff, 0xffff, 0xffff, 0xffff, 0x7fff))
XCTAssertEqual(highBit - otherBits, Decimal(1))
XCTAssertEqual(otherBits + Decimal(1), highBit)
}

func test_Misc() {
Expand Down Expand Up @@ -478,6 +506,10 @@ class TestDecimal: XCTestCase {
XCTAssertEqual(1, f._isCompact)
let after = f.description
XCTAssertEqual(before, after)

let nsd1 = NSDecimalNumber(decimal: Decimal(2657.6))
let nsd2 = NSDecimalNumber(floatLiteral: 2657.6)
XCTAssertEqual(nsd1, nsd2)
}

func test_PositivePowers() {
Expand Down