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

Remove unnecessary seed modifcation #73

Merged
merged 1 commit into from
Apr 27, 2024
Merged

Conversation

ironhaven
Copy link
Contributor

Because the first step of wyrand is an addition with a constant, a non-zero seed is not required

Because the first step of wyrand is an addition with a constant, a non-zero seed is not required
@notgull
Copy link
Member

notgull commented Feb 29, 2024

This slipped through the cracks. I'm not familiar enough with wyrand to know if this improves or doesn't improve randomness.

@Bluefinger
Copy link
Contributor

This slipped through the cracks. I'm not familiar enough with wyrand to know if this improves or doesn't improve randomness.

This is a holdover from when fastrand was based on PCG, not WyRand. And the C reference implementation doesn't do anything to avoid the non-zero seed: https://github.com/wangyi-fudan/wyhash/blob/master/wyhash.h#L151

However, if we modify the seed, it will a breaking change as the output will no longer quite be the same per sequence. And probably, if we are to change the output, I would suggest updating the constants in fastrand to use the final v4.2 ones as defined in the C reference, which should help to improve the entropy quality somewhat (though that can be another PR).

Copy link
Member

@notgull notgull left a comment

Choose a reason for hiding this comment

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

Right, that makes sense

@notgull notgull merged commit 63175fa into smol-rs:master Apr 27, 2024
14 checks passed
@notgull notgull mentioned this pull request Apr 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants