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

Make NewRng::new() return Self without Result wrapper #302

Merged
merged 1 commit into from
Mar 15, 2018

Conversation

dhardy
Copy link
Member

@dhardy dhardy commented Mar 14, 2018

Advantage: more convenient way to call an fn which almost never fails.

Advantage: more convenient way to call an fn which almost
never fails.
@dhardy dhardy added X-enhancement E-question Participation: opinions wanted labels Mar 14, 2018
Copy link
Contributor

@pitdicker pitdicker left a comment

Choose a reason for hiding this comment

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

Looks good to me!

As you argued, new() is extremely unlikely to fail, a convenience function, and an alternative that does return errors is easily available.

@dhardy dhardy merged commit 1caee2a into rust-random:master Mar 15, 2018
@dhardy dhardy deleted the new branch March 15, 2018 08:42
@nagisa
Copy link
Contributor

nagisa commented Mar 15, 2018 via email

@dhardy
Copy link
Member Author

dhardy commented Mar 15, 2018

Do we need to use UFCS here? RNG::from_rng works just so long as SeedableRng is in scope, and I hope it's obvious to readers where from_rng comes from.

@nagisa
Copy link
Contributor

nagisa commented Mar 15, 2018

It wasn’t obvious to me (although I haven’t taken time to familiarise myself fully with new trait hierarchy yet), and first look at the recommendation seemed to imply that RNG is a concrete type, rather than a generic variable and that there is no way to generically construct a fallible rng without panicking anymore.

It seems to me that users who will be reading this comment will be operating in the generic context anyway, so their code will be something like this:

fn foo<R: NewRng>(...) -> Result<...> {
    let rng = R::new(); // Hmm, I *really* want to not panic here.
}

Then recommending to use SeedableRng::from_rng makes it pretty obvious what has to change (note that changing the bound from NewRng to SeedableRng in this case is prudent thing to do, even if not strictly necessary):

fn foo<R: SeedableRng>(...) -> Result<...> {
    let rng = R::from_rng(hmm)?; // great!
}

@dhardy
Copy link
Member Author

dhardy commented Mar 15, 2018

Ah, sorry, I should have left this open for comment longer. We have quite a lot of PRs at the moment.

I'm assuming actually that many people will not be using generic types, though sure, sometimes they will, so for example:

use rand::{Rng, NewRng, SmallRng};

// This will only fail if something is quite wrong with the platform;
// in this case better just to die anyway:
let mut rng = SmallRng::new();

But sure, your generic version works too.

pitdicker pushed a commit that referenced this pull request Apr 4, 2018
Make NewRng::new() return Self without Result wrapper
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.

3 participants