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

Rename SmallRng::from_thread_rng to from_rand_rng #1531

Closed
wants to merge 5 commits into from

Conversation

dhardy
Copy link
Member

@dhardy dhardy commented Nov 20, 2024

  • Added a CHANGELOG.md entry

Summary

Title

Motivation

Function thread_rng was renamed to just rng. We can't call this method from_rng since that clashes with the SeedableRng method, so this seems like the next best choice (also aligning somewhat with the terminology we use in docs).

@dhardy dhardy requested review from vks and newpavlov November 20, 2024 11:03
@dhardy
Copy link
Member Author

dhardy commented Nov 20, 2024

So in benchmarks we're now doing stuff like this:

    bench(&mut g, "chacha20", ChaCha20Rng::from_os_rng());
    bench(&mut g, "std", StdRng::from_os_rng());
    bench(&mut g, "small", SmallRng::from_rand_rng());

The reason we don't have from_rand_rng everywhere is because SeedableRng is defined in rand_core and rand::rng is not available there.

This leads me to conclude that perhaps:

  1. We should remove SmallRng::from_rand_rng which is a special case, and use RNG::from_rng(&mut rand::rng()) instead
  2. We should merge rand_core back into rand so that we can add SeedableRng::from_rand_rng. Major caveat: rand will not be able to depend on externally-defined-RNGs like StdRng currently does.

Obviously, the first solution looks better. from_thread_rng was added in #1368 where the SeedableRng impl was dropped, but that has since been added back in.

@dhardy dhardy mentioned this pull request Nov 20, 2024
1 task
@dhardy dhardy closed this Nov 20, 2024
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.

1 participant