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

Closed
wants to merge 3 commits into from
Closed

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. Change removed from this PR.

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 dhardy requested review from newpavlov and vks September 13, 2024 07:26
@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).

@dhardy dhardy force-pushed the inherent-rng-methods branch from 2a5de77 to 7c72ebd Compare September 24, 2024 09:50
@dhardy
Copy link
Member Author

dhardy commented Sep 24, 2024

This PR no longer removes the rand::random function (left to a future PR). Other changes of this PR I'd like to see merged, unless there's any issue I haven't spotted here. Ultimately I hope we can replace these inherent impls with native Rust support for inherent trait methods.

@dhardy dhardy marked this pull request as ready for review September 24, 2024 09:54
@dhardy dhardy added D-review Do: needs review and removed E-question Participation: opinions wanted labels Sep 24, 2024
Copy link
Member

@newpavlov newpavlov left a comment

Choose a reason for hiding this comment

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

Personally, I do not like duplication of trait methods as inherent. I think it obscures the code and make it less consistent across crates.

@dhardy
Copy link
Member Author

dhardy commented Sep 27, 2024

I want to agree, especially since these inherent impls will not work for any RNG outside of rand. The objective was to ease usage (esp. for people new to Rust), but the result making this less consistent across RNGs does not help.

dhardy added a commit that referenced this pull request Oct 1, 2024
This extracts the non-inherent-methods stuff from #1492.
@dhardy dhardy force-pushed the inherent-rng-methods branch from a7ddabf to 9af9444 Compare October 3, 2024 07:49
@dhardy dhardy closed this Oct 7, 2024
@dhardy dhardy deleted the inherent-rng-methods branch October 16, 2024 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D-review Do: needs review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants