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

Inherent rng methods #1492

Draft
wants to merge 12 commits into
base: master
Choose a base branch
from
Draft

Conversation

dhardy
Copy link
Member

@dhardy dhardy commented Sep 10, 2024

  • Added a CHANGELOG.md entry

Summary

Make Rng methods inherent methods on RNGs, and more usability tweaks.

Motivation and details

This follows arguments made in #989 and #1488. Specifically,

  • We rename Rng::gen_iter to random_iter since it is the iterable version of Rng::random
  • We implement all (non-deprecated) Rng methods on StdRng, SmallRng, ThreadRng and StepRng
  • We remove rand::random, favouring usage of rand::thread_rng().random instead. This is not necessary but is more in-line with other Rng methods.

I did consider publicly exporting impl_rng_methods_as_inherent, but realised a problem: rand_chacha cannot use this since rand depends on rand_chacha (and the macro cannot be moved to rand_core); some other RNG crates could use this but should not depend on rand.

Incomplete / questions

Do we want to rename gen_range to just range? What about gen_bool, gen_ratio?

Do we want to remove rand::random? If not, do we want to add random_iter, gen_range etc. as free functions? (rand::random is hardly used inside rand itself. This GitHub search has 33k code hits but it's hard to tell how many are genuine matches.)

@oconnor663 gave motivation to rename thread_rng to just rng:

you [the user] have to understand what thread_rng means and that it's the right thing to use

We could do this. If this were a clean design then probably we should: it's an implementation detail that we use a thread-local generator and not, say, a mutex over a single (static) generator. But it's also rather widely used: approx 150 mentions in this repo and 176k code hits via GitHub search.

@dhardy dhardy added the E-question Participation: opinions wanted label Sep 11, 2024
@dhardy
Copy link
Member Author

dhardy commented Sep 13, 2024

I was intending to leave these changes until after rand v0.9, but now think we should get these merged first: upgrading to 0.9 already brings many small breaking changes for users; rolling these into a single (larger) changeset should result in less work for those upgrading.

Some more thoughts:

  • Renaming thread_rngrng is an easy search+replace (no false positives likely). Is there a downside (e.g. the possibility of adding another RNG)? We've stuck with this rough design for several years now (but changing the algorithm, seeding, access control).
  • We have Rng::gen_range which uses Uniform... weird but okay (though we could hypothetically rename to Rng::uniform and add rand::uniform).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-question Participation: opinions wanted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant