-
Notifications
You must be signed in to change notification settings - Fork 429
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
dhardy
wants to merge
12
commits into
rust-random:master
Choose a base branch
from
dhardy:inherent-rng-methods
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Inherent rng methods #1492
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Looks like a mistake in rust-random#1490.
Move gen_range down
I was intending to leave these changes until after Some more thoughts:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
CHANGELOG.md
entrySummary
Make
Rng
methods inherent methods on RNGs, and more usability tweaks.Motivation and details
This follows arguments made in #989 and #1488. Specifically,
Rng::gen_iter
torandom_iter
since it is the iterable version ofRng::random
Rng
methods onStdRng
,SmallRng
,ThreadRng
andStepRng
rand::random
, favouring usage ofrand::thread_rng().random
instead. This is not necessary but is more in-line with otherRng
methods.I did consider publicly exporting
impl_rng_methods_as_inherent
, but realised a problem:rand_chacha
cannot use this sincerand
depends onrand_chacha
(and the macro cannot be moved torand_core
); some other RNG crates could use this but should not depend onrand
.Incomplete / questions
Do we want to rename
gen_range
to justrange
? What aboutgen_bool
,gen_ratio
?Do we want to remove
rand::random
? If not, do we want to addrandom_iter
,gen_range
etc. as free functions? (rand::random
is hardly used insiderand
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 justrng
: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.