Skip to content

bump rand to 0.9.0-beta.3; renamed "gen" to "random"; test bigrand; fix test lints #317

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

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

Conversation

bionicles
Copy link

just went through and applied clippy suggestions

@bionicles bionicles changed the title fix clippy lints on tests bump rand to 0.9.0-beta.3; test bigrand module; fix clippy lints on tests Jan 21, 2025
@bionicles
Copy link
Author

i increased the minor version of the rand dependency
renamed "gen" to "random" in bigrand (note: this will be a breaking change, but it's necessary because of the new gen keyword being reserved)
refactored the implementations to match the updated trait
added basic tests for rand feature working (inside bigrand.rs module)

@bionicles bionicles changed the title bump rand to 0.9.0-beta.3; test bigrand module; fix clippy lints on tests bump rand to 0.9.0-beta.3; renamed "gen" to "random"; test bigrand; fix test lints Jan 21, 2025
@cuviper
Copy link
Member

cuviper commented Jan 21, 2025

If you want me to consider the clippy fixes now, please keep that in a distinct PR. Doing too much in one PR is a pretty sure way to make sure none of it gets pulled in -- especially for a first-time contributor.

The rand stuff is a breaking change even if you hadn't changed any names, because that's a public dependency. I also already have my own local branch for this upgrade, but I will wait for the proper rand 0.9.0.

@bionicles
Copy link
Author

ok, not sure how to split up a PR, ill look into it, my bad

@atouchet
Copy link

rand 0.9 is now out.

@bionicles
Copy link
Author

Thank you for making this awesome crate, thank you for your time, mainly just want to take another look here so we could support rand 0.9 and then I wouldn't need to use my local version of this crate anymore cuz that makes CI/CD annoying

I really like this crate and use it for something critical to my project, just went back to it and checked, tests are passing, just feeling curious i turned on the "pedantic" and "nursery" lint groups and found a few random subtle issues ... here are pics of some of the lints

to check this out locally, just add

[lints.clippy]
pedantic = "warn"
nursery = "warn"

to the bottom of Cargo.toml

(all of the other groups are fine)

i've experienced major 9x speedups from applying pedantic / nursery lints in the recent past, could be worth the time to take a look, but I already made a lot of changes in a single PR here, and I'm not sure how you'd want to deal with try_from so I couldn't fix everything

Observations:

could it be worth our time to review the linter suggestions around as conversions?

image

do "as conversions" turn negative numbers into positive ones? that could be unexpected, certainly doesn't make sense to me

image

i noticed some of the "from" implementations use macros, however could the 8/16/32 bit primitive conversions use "from" instead of "as" whereas the usize and isize primitives do need "as" or "try_from"? is that a correctness thing or just readability?

image

seems like most of the clippy issues are due to the as conversions and fixing them would require returning "result"

image

these last few are the kind that could yield performance benefits

image

image

image

could make some functions const and potentially enable new uses

image

some of that stuff i didn't even know existed!

anyway, none of this is strictly necessary to fix, maybe the "as conversions" could be worth a look as they could create some unexpected math voodoo people aren't aware of. mainly just want to bump this thread so we could support latest rand and ditch gen keyword. please let me know if you want more changes or a new more focused PR

finally, one dumb question, I noticed in my use case when I converted a Vec to Arc<[usize]> i got a massive 70% speedup, do you think the BigUint could use Arc<[BigDigit]> instead of Vec for similar gains (mostly from cheap clones?)

thanks for your work

@yhx-12243 yhx-12243 mentioned this pull request Mar 8, 2025
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.

3 participants