Fix asInt conversion and test across Int8-128 & UInt8-UInt128#12
Open
kdubb wants to merge 1 commit into
Open
Conversation
To workaround BInt only supporting `asInt() -> Int` (none generic and native size only). I created `asFixedWidthInteger()` in `Extensions.swift`. This functions grabs the bytes for the BInt and copies them into an integer of the appropriate size with appropriate sign conversions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The current implementation of
asInt<I:FixedWidthInteger>()is currently broken. Amongst other issues, it fails to convertBigDecimal(0)andBigDecimal(1).I fixed the implementation by:
It now uses a rounded (via truncation) value and then uses a simple
truncated != selfcomparison to reject fractions.asFixedWidthInteger()extension function toBIntUses
BInt.asMagnitudeBytes()and sign conversion to build an integer of the appropriate size.BInt.asIntvsasFixedWidthInteger()Adding the
asFixedWidthInteger()is necessary becauseBIntonly supportsasInt() -> Int(non-generic and native size only). Obviously this causes issues with integer types that have a range that differs fromInt.asFixedWidthInteger()instead usesBInt.asMagnitudeBytes()to export the bytes and copies them to an integer of the appropriate size, after which it modifies the sign as appropriate. Ultimately this, or a similar generic function, should probably be transferred to theBIntpackage.With the conversion from
BIntto anyFixedWidthIntegerencapsulated in an extension function, and because I'm usingasMagnitudeBytes()to do the byte export fromBInt, I was able to reduce the # of operations to a singlewithExponent(0, .towardZero)plus the range checks.Warning
An issue with conversion from Swift's
Int128toBigDecimalwas discovered, which is fixed in #11. Until that PR is merged, some of the Int128 tests will fail.Note
I used Swift Testing for the test suite for its native support of parameterized tests which made testing and debugging much easier.