[SR-6671] Decimal.leastNonzeroMagnitude, leastNormalMagnitude should have -128 exponent #1386
Conversation
|
@swift-ci please test |
|
This one is shared with the overlay so we should make coordinated changes there if we go forward with it. |
|
Can we add some tests for this to verify the expected behavior? |
Do you mean that these codes should be fixed at the same time? https://github.com/apple/swift/blob/49a8cf050304e3503960f0f1815afafe71dba28a/test/stdlib/TestDecimal.swift#L130-L133
Sure, please add them. |
|
OK, I'll add some tests. |
| XCTAssertEqual(try trySerialize([-Decimal.leastNonzeroMagnitude]), "[-0.0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001]") | ||
| XCTAssertEqual(try trySerialize([Decimal.leastNonzeroMagnitude]), "[0.0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001]") | ||
| XCTAssertEqual(try trySerialize([-Decimal.leastNonzeroMagnitude]), "[-0.00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001]") | ||
| XCTAssertEqual(try trySerialize([Decimal.leastNonzeroMagnitude]), "[0.00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001]") |
There was a problem hiding this comment.
These tests failed because of changes in this PR.
| @@ -101,31 +101,31 @@ public struct Decimal { | |||
|
|
|||
| extension Decimal { | |||
| public static let leastFiniteMagnitude = Decimal( | |||
| _exponent: 127, | |||
| _exponent: Int32(Int8.max), | |||
There was a problem hiding this comment.
What do you think of such expression change?
There was a problem hiding this comment.
What's going on here, and where did this property come from? "Magnitudes" are always non-negative, but this is a huge negative number. I don't know why this property even exists, but the name and documentation are extremely misleading.
(@takasek This is tangential to your change, but this is the first I've seen this).
There was a problem hiding this comment.
Sorry, I haven't been respond for long time...
Referring to the code, _exponent has a precondition to conform Int8. I intend to describe the precondition.
https://github.com/apple/swift-corelibs-foundation/blob/a40cc5cb3159bf64d70cb657131a01272dc988e3/Foundation/Decimal.swift#L22-L28
More fundamental solution is to change public var _exponent: Int32 into public var _exponent: Int8 but I have no idea of the impact.
|
@swift-ci Please test. |
|
Are the objections above understood? Do they still need correction? |
|
This seems to have gone stale. Is it still needed, or can it be closed? |
|
@swift-ci test |
|
Any problem still on this issue? |
|
@swift-ci test |
# Conflicts: # TestFoundation/TestDecimal.swift
|
This issue is yet stale. |
|
The Swift project moved the default branch to More detail about the branch update - https://forums.swift.org/t/updating-branch-names/40412 |
Fixes https://bugs.swift.org/browse/SR-6671 .