feat: Kotlin/Native implementation of BigDecimal and BigInteger #1200
Conversation
ianbotsf
added
the
no-changelog
gradle.properties
Outdated
| kotlin.incremental.js=true | ||
| kotlin.incremental.multiplatform=true | ||
| kotlin.mpp.stability.nowarn=true | ||
| kotlin.native.binary.sourceInfoType=libbacktrace |
There was a problem hiding this comment.
Were you able to get better stack traces with this enabled? I remember you said it didn't help, so I'd rather leave it off if it's not working as expected
There was a problem hiding this comment.
Good point, will remove. I eventually discovered that full stack traces are available in the build artifacts (e.g., build/reports/tests/linuxX64Test/index.html) just not in the console output.
|
|
||
| actual override fun equals(other: Any?): Boolean = other is BigDecimal && delegate == other.delegate | ||
| public actual override fun equals(other: Any?): Boolean = other is BigDecimal && delegate == other.delegate | ||
| public actual override fun hashCode(): Int = 31 + delegate.hashCode() |
There was a problem hiding this comment.
Usually 31 is multiplied in these hashCode implementations, is this intentional addition? Is there any reason we can't just take the delegate's hash code?
There was a problem hiding this comment.
Yes, I intentionally used addition instead of multiplication although either should work. I wanted to avoid using the unmodified hash code from the delegate because that would bin them the same if they were both added to a hashed container (e.g., Set). That's unlikely but also trivial to avoid.
| public actual operator fun plus(other: BigDecimal): BigDecimal = | ||
| coalesceOrCreate(delegate + other.delegate, this, other) | ||
|
|
||
| public actual operator fun minus(other: BigDecimal): BigDecimal = | ||
| coalesceOrCreate(delegate - other.delegate, this, other) |
| public actual override fun toString(): String = toString(10) | ||
| public actual fun toString(radix: Int): String = delegate.toString(radix) | ||
|
|
||
| public actual override fun hashCode(): Int = 17 + delegate.hashCode() |
There was a problem hiding this comment.
Same question about adding 17, can we just take the delegate's hash code?
| private companion object { | ||
| /** | ||
| * Returns a new or existing [BigDecimal] wrapper for the given delegate [value] | ||
| * @param value The delegate value to wrap | ||
| * @param left A candidate wrapper which may already contain [value] | ||
| * @param right A candidate wrapper which may already contain [value] | ||
| */ | ||
| fun coalesceOrCreate(value: IonSpinBigDecimal, left: BigDecimal, right: BigDecimal): BigDecimal = when (value) { | ||
| left.delegate -> left | ||
| right.delegate -> right | ||
| else -> BigDecimal(value) | ||
| } | ||
| } |
There was a problem hiding this comment.
I feel like this coalesceOrCreate might be a candidate for a generic function
There was a problem hiding this comment.
That'd be nice but our BigInteger and BigDecimal definitions don't have a common interface which defines val delegate as a public member. I'd rather not add one either since the delegate is an internal implementation detail.
This comment has been minimized.
This comment has been minimized.
ianbotsf
added
the
acknowledge-api-break
Affected ArtifactsChanged in size
|
3 participants
Issue #
(none)
Description of changes
This change implements Kotlin/Native support for
BigInteger/BigDecimalby way of the IonSpin kotlin-multiplatform-bignum library. This change adds a workaround for building smithy-kotlin on Amazon Linux 2 because of linking issues with zlib (see runtime/build.gradle.kts).By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.