Implement Decimal #659

alblue · 2016-09-27T09:51:25Z

Although not yet fully covered by tests, this is a nearly complete implementation of NSDecimal. There are cases where dealing with large and small numbers will overflow that isn't implemented yet (there is one remaining NSUnimplemented) but it can be used for simple mathematical operations.

alblue · 2016-09-27T15:56:20Z

Foundation/NSDecimal.swift

+
+    public init(_exponent: Int32, _length: UInt32, _isNegative: UInt32, _isCompact: UInt32, _reserved: UInt32, _mantissa: (UInt16, UInt16, UInt16, UInt16, UInt16, UInt16, UInt16, UInt16)){
+        self._mantissa = _mantissa
+        self.__exponent = Int8(_exponent & Int32(0xff))


There is a bug with this, in that negative exponents don't work as expected. It doesn't dial the tests but there needs some additional work before this goes through, as well as more tests.

Fixed in subsequent push

alblue

Changes are needed in the initialiser to handle negative exponents properly.

alblue · 2016-09-27T16:19:56Z

It looks like Int8(truncatingBitPattern:) is required here

xwu · 2016-09-29T01:48:00Z

Foundation/NSDecimal.swift

+    public init(sign: FloatingPointSign, exponent: Int, significand: Decimal) {
+        self.init(_exponent: Int32(exponent) + significand._exponent, _length: significand._length, _isNegative: sign == .plus ? 0 : 1, _isCompact: significand._isCompact, _reserved: 0, _mantissa: significand._mantissa)
+    }
+    public init(signOf: Decimal, magnitudeOf magnitude: Decimal) {         self.init(_exponent: magnitude._exponent, _length: magnitude._length, _isNegative: signOf._isNegative, _isCompact: magnitude._isCompact, _reserved: 0, _mantissa: magnitude._mantissa)


Nit: missing newline before self.init

Can you explain what you mean? Self.init is on line 143 and the public init is on line 142.

Ah, sorry - the comment was on line 145.

xwu · 2016-09-29T01:50:59Z

Foundation/NSDecimal.swift

 }

-public func NSDecimalIsNotANumber(_ dcm: UnsafePointer<Decimal>) -> Bool { NSUnimplemented() }
-
+public func NSDecimalIsNotANumber(_ dcm: UnsafePointer<Decimal>) -> Bool { return dcm.pointee.isNaN


Nit: missing new line before return

Fair enough, can fix

xwu · 2016-09-29T01:52:42Z

Foundation/NSDecimal.swift

+                _isCompact = 1
+            } else {
+                _isCompact = 0
+            }


Nit: could this be more terse? _isCompact = newValue ? 1 : 0

Yes, it could be but I don't know if there's aversion to using a ternary operator.

xwu · 2016-09-29T01:55:21Z

Foundation/NSDecimal.swift

+        // Divide by 10 as much as possible
+        // Put the non-empty remainder in place
+        // Set the new exponent
+        isCompact = true


I'm confused by this stub. It doesn't do anything, so why not leave NSDecimalCompact(_:) unimplemented?

Mainly because the NSDecimalCompact call is invoked from the init(Double) constructor, without which you couldn't test this. However that part of it is still a work in progress, so this could be left out of this patch set for now.

phausler · 2016-09-29T16:02:53Z

Foundation/NSDecimal.swift

+                UInt8(UInt32(newValue & (0b11 << 16)) >> 10)
+            __reserved = UInt16(newValue & 0b1111_1111_1111_1111)
+        }
+    }


these accessors change the structural layout and size of Decimal, it will probably make it incompatible when archiving and serializing it. Is there a compelling reason to re-layout the structure? To port addition and division and compaction it might be nice to be able to send these over to C functions.

These bring the structure to be the same format as the Objective-C structure. At the moment both Linux and Objective-C are different. Secondly, there is a difference in behaviour visible, in that the results are truncated on Darwin but not on Linux (hence the tests for isCompact:3). That's because the structure on the Objective-C is packed. Note that I have run these tests on both Linux and Objective-C to verify the same behaviour. Trivially, on Darwin at the moment:

1> import Foundation
2> MemoryLayout.size
$R0: Int = 20

and on Linux:

1> import Foundation
2> MemoryLayout.size
$R0: Int = 36

I would argue that to be performant we would want to have these as the same data structure.

Hmm i guess the importer is claiming them to be different sizes than the actual bit-packed size. So i guess your solution is probably the best we can do for now sadly.

phausler · 2016-09-29T16:07:51Z

Foundation/NSDecimal.swift

+    }
+    public static func ==(lhs: Decimal, rhs: Decimal) -> Bool {
+        if lhs.isNaN || rhs.isNaN {
+            return false


To match the behavior of the objective-c version:

// NaN is a special case and is arbitrary ordered before everything else // Conceptually comparing with NaN is bogus anyway but raising or // always returning the same answer will confuse the sorting algorithms if(NSDecimalIsNotANumber(leftOperand)) { if(NSDecimalIsNotANumber(rightOperand)) { return NSOrderedSame; } return NSOrderedAscending; }

so if both are NaN then they are ordered same which is considered equal (irrespective of any other behavior standard of floating point numbers)

OK, can fix that

By the way there's some inconsistency going on here because of this. On Darwin:

1> import Foundation
2> let a = Decimal.nan
3> let b = Decimal.nan
4> a.isEqual(to:b)
$R0: Bool = true
5> a.isTotallyOrdered(belowOrEqualTo: b)
$R1: Bool = false

You also have the fact that:

6> Double.nan == Double.nan
$R2: Bool = false

which makes it doubly confusing, though slightly off-topic for here.

I'm happy to implement/make this into bug compatible with Darwin but arguably the fact that you have a different behaviour between 'isEqual' and 'isTotallyOrdered' is a little strange.

phausler · 2016-09-29T16:09:22Z

Foundation/NSDecimal.swift

+        if lhs.isNaN || rhs.isNaN {
+            return false
+        }
+        if lhs.__exponent == rhs.__exponent && lhs.__lengthAndFlags == rhs.__lengthAndFlags && lhs.__reserved == rhs.__reserved {


negative values fall into here?

The 'lengthAndFlags' field contains both the length (upper nibble) and the compact/negative flags. So this will test for negative as well. We could explicitly test the flags individually but then we're doing a lot of byte wrapping for no real reason. The 'lengthAndFlags' looks like:

llllxxxx
xxxxCxxx
xxxxxNxx

got it; perhaps worth either a comment or a ivar name change to reflect that it it stored there

Any suggestions as to what to call it? __lengthAndNegativeAndCompact?

phausler · 2016-09-29T16:09:37Z

Foundation/NSDecimal.swift

+        }
+        var lhsVal = lhs
+        var rhsVal = rhs
+        return NSDecimalCompare(&lhsVal, &rhsVal) == .orderedSame


The Darwin version just calls this.

Sorry, I don't understand the comment on its own. Is it related to other comments or code paths?

phausler · 2016-09-29T16:11:00Z

Foundation/NSDecimal.swift

+    destination.pointee.__lengthAndFlags = source.pointee.__lengthAndFlags
+    destination.pointee.__exponent = source.pointee.__exponent
+    destination.pointee.__reserved = source.pointee.__reserved
+    destination.pointee._mantissa = source.pointee._mantissa


The objective-c version does not preserve reserved (but perhaps it should)

I'm happy to just leave setting reserved to a no-op and treating them as zero if you want me to change the Swift version

The only part that might matter is for verification of behavior. To API consumers it ought to not matter.

phausler · 2016-09-29T16:12:05Z

Foundation/NSDecimal.swift


-public func NSDecimalCompact(_ number: UnsafeMutablePointer<Decimal>) { NSUnimplemented() }
+public func NSDecimalCompact(_ number: UnsafeMutablePointer<Decimal>) {
+    number.pointee.compact()


This is upside down from the objective-c implementation, for consistency sake we might want to put the workhorse logic here instead of member functions (and perhaps treat the NSDecimal* functions as "implementation details"

Using the UnsafeMutablePointer is actually pretty painful. You can't (for example) have 'var d = number.pointee; d.doStuff()' because it ends up with a different copy. So everywhere you have the functions you need to do number.pointee to use the passed in version. In addition, it's not being safe for the sake of it, but rather adopting an older API.

Finally you can't tell what is being mutated and what isn't with these APIs; you're just passing an opaque pointer. This is tedious when doing non-mutating code (e.g. the NSDecimalIsNotANumber) because you have can't pass in a 'let' constant:

1> import Foundation
2> let one = Decimal(1)
one: Decimal = {
_mantissa = {
0 = 8448
1 = 0
2 = 1
3 = 0
4 = 0
5 = 0
6 = 0
7 = 0
}
}
3> NSDecimalIsNotANumber(one)
error: repl.swift:3:23: error: cannot convert value of type 'Decimal' to expected argument type 'UnsafePointer'
NSDecimalIsNotANumber(one)
^~~

3> NSDecimalIsNotANumber(&one)
error: repl.swift:3:23: error: cannot pass immutable value as inout argument: 'one' is a 'let' constant
NSDecimalIsNotANumber(&one)
^~~~

Having those as the API is probably a bad thing to do; in fact, I'd argue that the NSDecimalXxxx functions should be marked as deprecated. Right now the simple (non-mutating) ones can be more efficiently implemented (read: not taking an unsafe pointer) by doing it the reverse way from the way that Objective-C did it, and having the functions as backwards compatible for those that need it.

We can't mark them as deprecated and still use them to power the extended methods on NSDecimal (perhaps we could rename them so that they are not publicly autocompleted; but that is not going to happen likely for Swift 3.X).

Often when making the overlay I had to use a temp copy to transact upon instead. And we can't make them hoisted since they transact upon an address of a Decimal not the Decimal itself.

The Decimal type on Darwin is a packed structure, and so the implementation in Swift needs to follow the same packed layout to ensure that the values are truncated in the same way. Add test coverage for initialisers and verify the same test code passes on Swift on Darwin with the same results.

This adds implementations and test coverage of Decimal which passes on both Darwin and Linux.

Provides implementations of multiply and power, as well as a higher-precision Decimal equivalent that is used for implementing mathematical operations internally.

alblue · 2016-10-13T10:05:32Z

This isn't yet complete (test coverage and normalisation NSUnimplemented still to go) but is in a state where the content can be seriously reviewed ahead of merging.

alblue · 2016-10-13T10:28:04Z

Foundation/NSDecimal.swift

+            return error;
+        }
+
+        for i in 0..<Decimal.maxSize {


This should probably be big._length rather than Decimal.maxSize, otherwise we'll lose precision

alblue · 2016-10-13T10:32:37Z

Foundation/NSDecimal.swift

+    //
+    var backup = Decimal()
+
+    NSDecimalCopy(&backup,aa)


NB this seems redundant at the moment but will be used in the NSUnimplemented below

alblue · 2016-10-13T10:33:06Z

Foundation/NSDecimal.swift

+                result.pointee._exponent = newExponent
+            }
+            result.pointee._length = big._length
+            for i in 0..<Decimal.maxSize {


This should be big._length as well, I think

alblue · 2016-10-13T10:34:06Z

Foundation/NSDecimal.swift

+        newExponent += exponent
+    }
+
+    for i in 0..<Decimal.maxSize { // bigSize?


Think this should be big._length as well

alblue · 2016-10-13T10:35:03Z

Foundation/NSDecimal.swift

+    var isZero:Bool { get }
+}
+
+extension Decimal: VariableLengthNumber {


Not sure if we can apply fileprivate here but we should if we could

Nope, compile time error to try. Still, VariableLengthNumber is an internal protocol so hopefully it won't be visible.

alblue · 2016-10-13T10:35:39Z

Foundation/NSDecimal.swift


-public func NSDecimalMultiplyByPowerOf10(_ result: UnsafeMutablePointer<Decimal>, _ number: UnsafePointer<Decimal>, _ power: Int16, _ roundingMode: NSDecimalNumber.RoundingMode) -> NSDecimalNumber.CalculationError { NSUnimplemented() }
+// == Internal (Swifty) functions ==


It would be good to make these public API and have the same on the Darwin SDK - thoughts?

That would have to be something that we would need to pass through API review and decide on if they have the merit to maintain etc. Can they be re-written to use the free floating functions? it seems like there is some duplication of effort here.

Ideally we should have these to be sharable code from swift-corelibs-foundation to the Darwin overlay for Foundation.

The integer division requires the creation of a new protocol to unify WideDecimal and Decimal. This has been referred to as VariableLengthNumber, because it builds upon the functions in CoreFoundation that used NSInteger built-in functions, but without conflating the Swift Integer type.

xwu · 2016-10-13T13:36:28Z

Foundation/NSDecimal.swift

 }

-public func NSDecimalIsNotANumber(_ dcm: UnsafePointer<Decimal>) -> Bool { NSUnimplemented() }
+fileprivate func divideByShort<T:VariableLengthNumber>(_ d: inout T, _ divisor:UInt16) -> (UInt16,NSDecimalNumber.CalculationError) {


Divide and carry, multiply and carry, etc, are best off using the new double-width divide and multiply facilities now available with new integer protocols. Those were exposed precisely for implementation of types such as Decimal.

Shouldn't these be internal methods on Decimal, not free functions?

These were based on the implementation from Core Foundation for ease of review and acceptance. I am fine with the implementations being modified subsequently to use alternative methods of calculation. I would caution, though, that the NSDecimal* top level functions are public API and are a real nightmare to implement as sane methods on Decimal, so it is not trivial to do as such.

When we added functionality to Decimal to have methods it became quite clear that it is nearly impossible to mimic the behavior and avoid needing to call out to the freestanding functions. They will most likely need to remain API in swift to service the hoisted method implementations. The divideByShort and fellow methods are relatively complex and hold some quirky behavior that these seem to attempt to mimic - we should consider porting some of the edge case tests from Foundation to swift-corelibs

xwu · 2016-10-13T13:38:32Z

Foundation/NSDecimal.swift

+}
+
+fileprivate extension UInt16 {
+    func compareTo(_ other: UInt16) -> ComparisonResult {


Why do you need this?

To make it easier to implement the code

It looks like it's used only once and can be trivially inlined. Doing so would have performance benefit and improve clarity at the use site.

xwu · 2016-10-13T13:40:23Z

Foundation/NSDecimal.swift

+        // This formula and test for q gives at most q+1; See Knuth for proof.
+
+        let ul = u._length
+        let uu = u // work around compiler bug


What compiler bug is this?

"Cannot subscript inout parameter u of type Decimal"

might be good to attach bugs.swift.org url here for a filed ticket just so it is known what bug it is

On what line are you encountering this error? You are clearly subscripting u successfully elsewhere, so something must be different about a particular use site where the argument is inout. Can you point that out and file a bug?

xwu · 2016-10-13T13:42:41Z

Foundation/NSDecimal.swift

+     * individual operands, but often produces a more
+     * accurate result later. I chose 19 arbitrarily
+     * as half of the magic 38, so that normalization
+     * doesn't always occur. */


This seems incorrect. What is the behavior of Darwin Foundation?

This is the comment from Darwin Foundation.

xwu · 2016-10-13T13:43:53Z

Foundation/NSDecimal.swift

+    if r._length != 0 {
+        r.negate()
+    }
+    return NSDecimalAdd(result, leftOperand, &r, roundingMode)


If you negate the operand, then the rounding mode may change.

The implementation is based on Foundation

phausler

Overall this looks pretty good.

phausler · 2016-10-13T16:42:04Z

Foundation/NSDecimal.swift

+        return .noError;
+    }
+
+    NSUnimplemented() // work in progress


might be good to comment on the remaining work here that needs to be done as a reminder ;)

phausler · 2016-10-13T16:46:41Z

Foundation/NSDecimal.swift


-public func NSDecimalMultiplyByPowerOf10(_ result: UnsafeMutablePointer<Decimal>, _ number: UnsafePointer<Decimal>, _ power: Int16, _ roundingMode: NSDecimalNumber.RoundingMode) -> NSDecimalNumber.CalculationError { NSUnimplemented() }
+// == Internal (Swifty) functions ==


That would have to be something that we would need to pass through API review and decide on if they have the merit to maintain etc. Can they be re-written to use the free floating functions? it seems like there is some duplication of effort here.

Ideally we should have these to be sharable code from swift-corelibs-foundation to the Darwin overlay for Foundation.

phausler · 2016-10-13T16:57:46Z

@swift-ci Please test and merge

alblue commented Sep 27, 2016

View reviewed changes

parkera assigned phausler Sep 27, 2016

xwu reviewed Sep 29, 2016

View reviewed changes

phausler reviewed Sep 29, 2016

View reviewed changes

alblue added 3 commits October 12, 2016 17:43

Add implementations of methods in Decimal

b8bcb84

This adds implementations and test coverage of Decimal which passes on both Darwin and Linux.

Continued implementation of NSDecimal

f2dffa1

Provides implementations of multiply and power, as well as a higher-precision Decimal equivalent that is used for implementing mathematical operations internally.

alblue changed the title ~~Add initialisers and test coverage for Decimal~~ Implement Decimal Oct 13, 2016

alblue commented Oct 13, 2016

View reviewed changes

xwu reviewed Oct 13, 2016

View reviewed changes

phausler approved these changes Oct 13, 2016

View reviewed changes

swift-ci merged commit cafc428 into swiftlang:master Oct 13, 2016


		public func NSDecimalMultiplyByPowerOf10(_ result: UnsafeMutablePointer<Decimal>, _ number: UnsafePointer<Decimal>, _ power: Int16, _ roundingMode: NSDecimalNumber.RoundingMode) -> NSDecimalNumber.CalculationError { NSUnimplemented() }
		// == Internal (Swifty) functions ==

Implement Decimal #659

Implement Decimal #659

Uh oh!

Conversation

alblue commented Sep 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alblue left a comment

Choose a reason for hiding this comment

Uh oh!

alblue commented Sep 27, 2016

Uh oh!

xwu Sep 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xwu Sep 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xwu Sep 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phausler Sep 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phausler Sep 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alblue commented Sep 27, 2016 •

edited

Loading

xwu Sep 29, 2016 •

edited

Loading

xwu Sep 29, 2016 •

edited

Loading

xwu Sep 29, 2016 •

edited

Loading

phausler Sep 29, 2016 •

edited

Loading

phausler Sep 29, 2016 •

edited

Loading