Skip to content
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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Test on i386 #141

wants to merge 2 commits into from

Conversation

konsumlamm
Copy link
Contributor

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 for r.rand(x..x).

Update CI
runs-on: ubuntu-latest
strategy:
fail-fast: false
matrix:
nim-version:
- '1.4.8'
Copy link
Contributor

@dlesnoff dlesnoff Dec 4, 2023

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't mind that, 1.4 has quite some bugs (#59, #93, #94) and doesn't receive backports anymore.

Copy link
Member

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.

tests/trandom.nim Outdated Show resolved Hide resolved
Comment on lines +40 to +42
let hi64 = r.rand(hi64Max)
limbs.add(uint32(hi64 and uint32.high))
limbs.add(uint32(hi64 shr 32))
Copy link
Contributor

@dlesnoff dlesnoff Dec 4, 2023

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.

Copy link
Contributor Author

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Why is this library not expected to work on 32 bit?
3 participants