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

Reorganisation of Rand items #435

Merged
merged 8 commits into from
May 11, 2018
Merged

Reorganisation of Rand items #435

merged 8 commits into from
May 11, 2018

Conversation

dhardy
Copy link
Member

@dhardy dhardy commented May 9, 2018

This is an alternative to #430. Only the last two commits make any changes to documentation (other than adding a single line in new modules), so more documentation work is required.

Notably now:

  • StdRng, SmallRng and ThreadRng are only available from the rngs module
  • EntropyRng and OsRng are only available from the entropy module
  • there is a small mis-match in that JitterRng still has its own module (within entropy) but this doesn't seem like a big deal
  • ReseedingRng is only available in reseeding; it's not going to be used directly much so lets hide it
  • new rand_core::block module
  • BlockRngCore is no longer exported from rand
  • prelude

Adding the prelude is an extra feature; it's not necessary but is nice in some ways. It doesn't contain much from the distributions module and omits Error because including generic names in the prelude might cause problems.

I also considered moving Error and ErrorKind into the error module (from the POV of doc and exports, both in rand and rand_core), which is neater but probably more inconvenient.

@dhardy dhardy added E-question Participation: opinions wanted B-API Breakage: API P-high Priority: high D-review Do: needs review labels May 9, 2018
src/lib.rs Outdated
@@ -804,8 +810,8 @@ pub trait FromEntropy: SeedableRng {
/// errors, use the following code, equivalent aside from error handling:
///
/// ```rust
/// # use rand::Error;
/// use rand::{Rng, StdRng, EntropyRng, SeedableRng};
/// use rand::{Error, prelude::*};
Copy link
Member Author

Choose a reason for hiding this comment

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

I should keep the old # use ...

/// [`SmallRng`]: rngs/struct.SmallRng.html
#[deprecated(since="0.5.0", note="removed in favor of SmallRng")]
#[cfg(feature="std")]
pub fn weak_rng() -> XorShiftRng {
Copy link
Member Author

Choose a reason for hiding this comment

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

Diff is messed up; this didn't change (only random pasted below and 2 Rngs removed)

@pitdicker
Copy link
Contributor

I will have a good look tomorrow.

@pitdicker
Copy link
Contributor

Two things stand out to me as really nice: the prelude (and what is included in it) and separating out BlockRng. And you have split the commits nicer.

But for the rest I think #430 looks much cleaner, sorry... But we are having trouble coming closer in how we look at things.

In a way it matters little how it all is organised. The most important outcome is presenting something easy and logical to users in documentation and imports. Having less modules to traverse seems like a win to me then, unless things are really not related.

I don't really like the name entropy, but can get behind a module named something like external_rngs (you suggested external before). The more we use entropy as a name, the more I feel we are drifting from the real meaning. I also wouldn't call them true RNGs, as that is questionable to say about OsRng. I am not sure why you would want to keep JitterRng in a module of its own.

I still see no problem moving ReseedingRng, ReadRng and mock::StepRng into the rngs module. I guess I look more at the similarities than the differences. As there seems no 'best' solution, can we make a compromise?

BlockRngCore is no longer exported from rand

Wasn't the argument that it is used in the public API of ReseedingRng? But I suppose that matters little, and it was a bit out of place in Rand.

@dhardy
Copy link
Member Author

dhardy commented May 10, 2018

You are right about the mis-use of the word "entropy"; we already have EntropyRng which is in the same vein as the new module, but in both cases it's not correct usage of the word. Good point about "true" being inaccurate for OsRng too.

JitterRng has its own error type; I think that is the only reason for the extra module.

So, what to do? I kind of like the std organisation: mostly avoids deep hierarchies, puts similar things together, but doesn't shy away from new modules for other things; this isn't true of the collections module though of course. Also it doesn't attempt to walk you through all available features in the doc, but only points out the most used ones and lets you find the others should you need them.

Question, if you are okay with an external_rngs module what do you think about renaming EntropyRngExternalRng? Best left to another PR however.

Okay, fine, I guess we can put most stuff together, like std::collections. I'm not happy about read/reseeding just "mixing in" with the rest though.

@dhardy
Copy link
Member Author

dhardy commented May 10, 2018

Notes on further mis-uses of "entropy". One description of "entropy" is as a measure of disorder, which can imply external disorder. I don't think we're too far wrong with our usage

  • FromEntropy::from_entropy
  • JitterRng::gen_entropy — this doesn't make entropy, but just gathers it
  • we use OpenBSD's getentropy

These are almost exclusively for implementation of certain types of RNG,
therefore exposing BlockRngCore in rand is not useful.
@dhardy
Copy link
Member Author

dhardy commented May 10, 2018

Updated and fixed (again). @pitdicker happy?

The 'adaptor' module isn't the best name; trying to strike a middle ground and I guess it's okay (wrapper doesn't appeal).

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.

@pitdicker happy?

Completely 😄

@dhardy dhardy merged commit f0f1efe into rust-random:master May 11, 2018
@dhardy
Copy link
Member Author

dhardy commented May 11, 2018

What can I say but 🎉

@dhardy dhardy deleted the reorg branch May 11, 2018 10:13
@pitdicker pitdicker mentioned this pull request May 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-API Breakage: API D-review Do: needs review E-question Participation: opinions wanted P-high Priority: high
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants