unisonweb / base

Unison base libraries
https://share.unison-lang.org/@unison/base
18 stars 6 forks source link

Fix Natural.toNat for numbers > 0xFFFFFFFF #62

Closed hagl closed 2 years ago

hagl commented 2 years ago

The original code of Natural.toNat multiplies the upper and lower 32bits instead of adding them, so for any Natural > 0xFFFFFFFF the result is wrong.

Fixes https://github.com/unisonweb/base/issues/57

This PR basically changes the * to + and adds a test case.

Natural.toNat n =
  match digits n with
    Nonempty a [b] ->
      use Nat +
-       Some (a * Nat.shiftLeft b 32)
+       Some (a + Nat.shiftLeft b 32)
    _              -> None

Ideally this bug should have been discovered by the existing property test Natural.toNat.tests.roundtrip. But with the generator gen.nat it only tests the cases from 0 to 999 unfortunately.

Btw. this is my first PR to a unison codebase. I tried to follow the instructions in CONTRIBUTING.md and hope everything is fine, but please check carefully if I missed anything

Code review

The changes summarized below are available for you to review, using the following command:

pull-request.load https://github.com/unisonweb/base:.trunk https://github.com/hagl/unison-workspace:.prs.base.issue_57

Updates:

 Natural.toNat : Natural -> Optional Nat
 ↓
 Natural.toNat : Natural -> Optional Nat
 +  unisoncomputing2021 : License
 +  authors.hagl        : Author
 +  hagl2021            : License
 +  runarorama          : Author

There were 4 auto-propagated updates.

 patch patch (added 1 updates)

Added definitions:

 Natural.toNat.tests.exampleMaxNat : [Result] (+4 metadata)
 metadata.authors.hagl.guid        : GUID
 metadata.authors.hagl             : Author
 metadata.copyrightHolders.hagl    : CopyrightHolder
 metadata.licenses.hagl2021        : License
runarorama commented 2 years ago

Thank you! Merged to trunk in hash #h9jjijvv53

🌈 ⭐