ionspin / kotlin-multiplatform-bignum

A Kotlin multiplatform library for arbitrary precision arithmetics
Apache License 2.0
366 stars 42 forks source link

Fix biginteger bitwise operations sign #142

Closed fullkomnun closed 3 years ago

fullkomnun commented 3 years ago

The sign of a bitwise xor operation's result was not calculated correctly. The sign of the result was set the sign of the first operand which resulted in

BigInteger.ONE xor BigInteger.ZERO == BigInteger.ONE while BigInteger.ZERO xor BigInteger.ONE == BigInteger.ZERO so xor is not commutative and the sign of the result depends on the order of operands.

I amended the logic of the xor output's sign so that:

  1. sign is Sign.NEGATIVE iff exactly one of the operands is negative (same as Java's BigInteger)
  2. Sign is Sign.ZERO iff magnitude of the result has value of 0
  3. else, sign is Sign.Positive

I also added a basic test for xor of a non-zero number with zero that makes sure it is commutative.

I added an assertion that guards against initialisation of BigInteger when sign == Sign.ZERO but magnitude does not have a value of zero or when magnitude does have a value of 0 but sign != Sign.ZERO.

NOTE: There could be more tests (maybe using generated data) to verify bitwise operators and sign of outputs of 'or' and 'and' needs to be verified as well.

CLAassistant commented 3 years ago

CLA assistant check
All committers have signed the CLA.

ionspin commented 3 years ago

Hi Or,

thank you for the pull request, it is really appreciated! I do have some comments that would need to be addressed first, especially since GitLab runners don't execute pipelines on pull requests from forked repos.

fullkomnun commented 3 years ago

Hi Or,

thank you for the pull request, it is really appreciated! I do have some comments that would need to be addressed first, especially since GitLab runners don't execute pipelines on pull requests from forked repos.

Sure

ionspin commented 3 years ago

Looks good, once again thanks for the contribution, it's greatly appreciated!

I'm going to push your commits to a new request so that GitLab CI can run the build on Mac and Windows, and I'm going to add just a small commit with a newline at the end of the test you added because spotless is complaining about it. I'll reference the new pull request here once I create it, and then I'll close this pull request

ionspin commented 3 years ago

143 with your commits is merged to master, in an hour or so once the build finished a new snapshot version will be released. I'll release a full version start of February, just to see if any other changes get in master by then.

If you want to use the snapshot version you'll have to add this:

repositories {
    maven {
        url = uri("https://oss.sonatype.org/content/repositories/snapshots")
    }
}
implementation("com.ionspin.kotlin:bignum:0.2.4-SNAPSHOT ")

to your build.gradle.kts

Once again, thanks for contributing!