Skip to content

Reorganisation of Rand items#435

Merged
dhardy merged 8 commits into
rust-random:masterfrom
dhardy:reorg
May 11, 2018
Merged

Reorganisation of Rand items#435
dhardy merged 8 commits into
rust-random:masterfrom
dhardy:reorg

Conversation

@dhardy

@dhardy dhardy commented May 9, 2018

Copy link
Copy Markdown
Member

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
dhardy
dhardy commented May 9, 2018
View reviewed changes
Comment thread src/lib.rs Outdated
/// ```rust
/// # use rand::Error;
/// use rand::{Rng, StdRng, EntropyRng, SeedableRng};
/// use rand::{Error, prelude::*};

@dhardy dhardy May 9, 2018

Copy link
Copy Markdown
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 ...

Comment thread src/lib.rs
/// [`SmallRng`]: rngs/struct.SmallRng.html
#[deprecated(since="0.5.0", note="removed in favor of SmallRng")]
#[cfg(feature="std")]
pub fn weak_rng() -> XorShiftRng {

@dhardy dhardy May 9, 2018

Copy link
Copy Markdown
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

pitdicker commented May 9, 2018

Copy link
Copy Markdown
Contributor

I will have a good look tomorrow.

@pitdicker

pitdicker commented May 10, 2018

Copy link
Copy Markdown
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

dhardy commented May 10, 2018

Copy link
Copy Markdown
Member Author

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 EntropyRng → ExternalRng? 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

dhardy commented May 10, 2018

Copy link
Copy Markdown
Member Author

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

dhardy added 3 commits May 10, 2018 10:31
@dhardy
Move all external RNGs to the rngs module
16f149e
@dhardy
Move ReadRng and ReseedingRng to new rngs/adaptor module
b9b2adc
@dhardy
Move BlockRng* to new rand_core::block module; don't export from rand
4b84f48 
These are almost exclusively for implementation of certain types of RNG,
therefore exposing BlockRngCore in rand is not useful.
@dhardy

dhardy commented May 10, 2018

Copy link
Copy Markdown
Member Author

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).

pitdicker
pitdicker approved these changes May 10, 2018
View reviewed changes

@pitdicker pitdicker left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@pitdicker happy?

Completely 😄

@pitdicker pitdicker mentioned this pull request May 10, 2018
Closed
@dhardy dhardy merged commit f0f1efe into rust-random:master May 11, 2018
@dhardy

dhardy commented May 11, 2018

Copy link
Copy Markdown
Member Author

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
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@pitdicker pitdicker pitdicker approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

B-API Breakage: API D-review Do: needs review E-question Participation: opinions wanted P-high Priority: high

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@dhardy @pitdicker