-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Test on i386 #141
base: master
Are you sure you want to change the base?
Test on i386 #141
Conversation
Update CI
runs-on: ubuntu-latest | ||
strategy: | ||
fail-fast: false | ||
matrix: | ||
nim-version: | ||
- '1.4.8' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to drop support for 1.4.8.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for dropping 1.4.x
It can also be part of a separate PR if you will. Don't forget to also bump minimal supported version in the .nimble
file.
let hi64 = r.rand(hi64Max) | ||
limbs.add(uint32(hi64 and uint32.high)) | ||
limbs.add(uint32(hi64 shr 32)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works by generating a 64-bit number. I am surprised that this works well on a 32-bit architecture.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why wouldn't it? 32 bit architectures support 64 bit numbers perfectly well, they're just slower (because they take up two memory locations).
Test on i386 (a 32 bit arch) in the CI and update the versions tested.
Closes #48.
The CI job uses the prebuilt binaries from https://nim-lang.org/install.html (since most actions aren't available in a container). This means we can finally confirm that the library works on 32 bit architectures. I had to fix an example and the
rand
code (to work around nim-lang/Nim#16360). I also added a test forr.rand(x..x)
.