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

Change RngCore doc in regards to Default #378

Closed
newpavlov opened this issue Apr 6, 2018 · 7 comments
Closed

Change RngCore doc in regards to Default #378

newpavlov opened this issue Apr 6, 2018 · 7 comments

Comments

@newpavlov
Copy link
Member

Currently we have the following line in the docs for RngCore:

also do not implement Default, but instead implement SeedableRng thus allowing use of rand::NewRng (which is automatically implemented)

I think it should be changed to "do not implement Default for pseudorandom generators" or to something similar. For hardware RNGs (e.g. rdrand) and OS RNGs it does not make sense to implement SeedableRng.

@vks
Copy link
Collaborator

vks commented Apr 10, 2018

Should we implement Default for OsRng and JitterRng, although it can panic?

@newpavlov
Copy link
Member Author

I think we should, especially considering that OsRng on most platforms is just ZST.

@pitdicker
Copy link
Contributor

OsRng::new can panic on at least on Linux, the generic implementation for Unix, and on WASM. We added all the stuff with error handling to make this clear and avoid panicking. I feel that implementing Default for OsRng would defeat this.

vks added a commit to vks/rand that referenced this issue Apr 10, 2018
@newpavlov
Copy link
Member Author

newpavlov commented Apr 10, 2018

It can only panic on pre-3.17 kernels, plus note that there is no requirement in the documentation that Default impl should never panic. (though I agree that it's a nice property to have) Also I do not suggest that we should remove new methods and replace them with Default trait.

BTW shouldn't we use unimplemented!() macro for WASM without stdweb instead of the current approach?

@pitdicker
Copy link
Contributor

there is no requirement in the documentation that Default impl should never panic.

I was going over things like the API guidelines to see if never panicking is recommended. And a bit surprising to me, but I can find no mention of the issue.

One other issue pointed out by @dhardy (somewhere) is that implementing Default with NewRng::new() (or whatever it is going to be called) does not really fit the crate organisation we have in mind. It would make Default the only thing that does not work with the traits and code in rand_core, but requires Rand and somewhat complex machinery like OsRng.

BTW shouldn't we use unimplemented!() macro for WASM without stdweb instead of the current approach?

I have no idea. There was a little discussion around that here #197 (comment).

@newpavlov
Copy link
Member Author

It would make Default the only thing that does not work with the traits and code in rand_core, but requires Rand and somewhat complex machinery like OsRng.

Could you please expand on this? Why implementing Default requires rand? For me Default is a perfectly stand-alone functionality which with SeedableRng represents two mutually exclusive ways of initializing RNG.

@pitdicker
Copy link
Contributor

I was thinking you wanted to implement Default with NewRng::new(), which is not provided by rand_core. Or did you have in mind to set the RNG to some constant seed?

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

No branches or pull requests

3 participants