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

Update SmallRng and remove rand_xorshift crate #623

Merged
merged 4 commits into from Oct 17, 2018

Conversation

dhardy
Copy link
Member

@dhardy dhardy commented Oct 4, 2018

This updates SmallRng as mentioned in #603.

rand_xorshift has been moved here.

@dhardy dhardy requested a review from vks October 4, 2018 18:12
@dhardy
Copy link
Member Author

dhardy commented Oct 5, 2018

Discussion of test failures: #624

@dhardy
Copy link
Member Author

dhardy commented Oct 15, 2018

Updated with a tiny bit of documentation (PRNGs module).

We still need to decide what to do with that module; possibly we could move the documentation here (as a new file). For 0.6 the prng module will still exist anyway (because of deprecations) but later it will be removed.

@dhardy
Copy link
Member Author

dhardy commented Oct 16, 2018

I should mention that this was rebased on #632 (which I merged without review since all content other than CI changes had been reviewed elsewhere and the move was already discussed in #624).

The changes here have also been discussed elsewhere (#603).

More documentation changes may be appropriate; I haven't had time to review that properly.

@dhardy dhardy mentioned this pull request Oct 16, 2018
28 tasks
src/prng/mod.rs Outdated
// 2. imperfect performance on tests or other limiting properties, but not
// terrible (e.g. Xoroshiro128+)
// 2. imperfect performance on tests or other limiting properties, or
// insufficient theory, but not terrible (e.g. SFC, Xoshiro, Xoroshiro128+)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I disagree with the example RNGs here. Maybe just remove them?

Copy link
Member Author

Choose a reason for hiding this comment

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

This stuff isn't in the printed doc (only two leading slashes). Yeah, it's difficult to know how to rate things; there's enough complexity to write a book on it or we could simply leave users with no hints at all. I already changed this rating system to demote PCG to 3 stars because I don't think it should be compared so closely with crypto RNGs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fair enough. The 4 and 5 star ratings are unused anyway, because the CSPRNG sections does not have a quality rating.

Copy link
Member Author

Choose a reason for hiding this comment

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

It could, but unless all PRNGs were put in a single table it wouldn't make much sense. Further comments on this in #633.

@vks
Copy link
Collaborator

vks commented Oct 16, 2018

The rest looks good to me.

@dhardy dhardy merged commit f786359 into rust-random:master Oct 17, 2018
@dhardy dhardy deleted the small-rng branch February 15, 2019 10:58
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.

None yet

2 participants