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

Add Default for ThreadRng #657

Merged
merged 2 commits into from
Dec 17, 2018
Merged

Conversation

burdges
Copy link
Contributor

@burdges burdges commented Dec 4, 2018

Appears harmless. I'm unsure if this actually helps anything, especially since doing this for OsRng requires unwrap, but RandomState: Default though so..

@dhardy
Copy link
Member

dhardy commented Dec 4, 2018

I guess this lets people derive(Default) if they want to do that. Not really sure whether including ThreadRng in a field is a good pattern, but I suspect it makes sense in some cases.

We opted to keep thread_rng #404 but I don't see any harm in adding this.

One suggestion: add brief documentation on this implementation and on fn thread_rng stating their equivalence.

src/rngs/thread.rs Outdated Show resolved Hide resolved
@dhardy
Copy link
Member

dhardy commented Dec 6, 2018

WASM fails us again 🙁

@burdges
Copy link
Contributor Author

burdges commented Dec 6, 2018

Oh? So thread_rng() works but not this? Odd.

@dhardy
Copy link
Member

dhardy commented Dec 6, 2018

I suspect the test is failing on master too?

I restarted it: https://travis-ci.org/rust-random/rand/builds/459690582

@dhardy
Copy link
Member

dhardy commented Dec 16, 2018

@burdges can you please rebase since #660 should fix the test.

@burdges
Copy link
Contributor Author

burdges commented Dec 16, 2018

We'll see if that works then.

I suppose if anyone needs OsRng: Default then they can wrap OsRng using whatever .unwrap() makes them happy. We might've delegation before that ever comes up. :)

@dhardy dhardy merged commit 2b5eac5 into rust-random:master Dec 17, 2018
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.

2 participants