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

In the unlikely event #15

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

In the unlikely event #15

wants to merge 2 commits into from

Conversation

tokenrove
Copy link
Owner

I clearly started renovating this a few years ago and then stopped. Probably should merge these changes, not that I think anyone still uses this library.

So, if we pick a modulus of 0, of course uniform_1 croaks.  If our
generator really is acceptably random, that should be pretty rare.  We
add another call, which still doesn't guarantee we don't get two zeros
in a row, but if that happens, the PRNG is more likely to be broken or
your luck was really bad.  We could just bitwise-or in a 1, but I
prefer having this canary.

As evidence that I have tested this code too infrequently without
rdrand, it turns out that PCG32 initialization without rdrand has been
quite weak, all this time, and it's quite common to get 0 from
uniform32 here as the modulus, much more than it should have been.
Without rdrand, benchmarks were aborting because the first value
generated was often zero.  Having high-quality seeding has never been
a goal of this library, but this was pretty bad.  So we mix in an
initialization constant (from some other code I wrote using pcg32,
which presumably took it from upstream PCG examples or a similar
source).
@tokenrove tokenrove marked this pull request as ready for review December 21, 2022 20:20
@lpgauth lpgauth mentioned this pull request Jun 3, 2023
@lpgauth
Copy link
Contributor

lpgauth commented Jun 6, 2023

FYI, I've tested these change and they seem fine.

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.

2 participants