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

Move all *Rng wrappers to rngs module #430

Closed
wants to merge 1 commit into from

Conversation

pitdicker
Copy link
Contributor

@pitdicker pitdicker commented May 6, 2018

This is the first commit of #423, moved out to help reviewing.

The idea is to move all *Rng wrappers to an rngs module, similar to how everything distribution-related is in the distributions module.

This gives a nice clean-up of our exports from the to level, which now includes many things, and some are things 99% of the users of Rand would not need. We had complaints about the number of items there before (can't remember where). And a separate module provides a nice place for more targeted documentation.

The RNGs moved to the new module are:

  • StdRng
  • SmallRng
  • ThreadRng
  • EntropyRng
  • JitterRng (including the public TimerError)
  • OsRng
  • ReadRng
  • ReseedingRng

Modules with only one type are no longer exposed, so no more os, jitter or read modules. The structure of the old modules is recreated using re-exports, but hidden in the documentation. That will give a problem in the future, do we want to remove those compatibility re-exports someday? As far as I know there is no way to give deprecation warnings.

I only kept StdRng and SmallRng as re-exports in the top-level module as items that should not be deprecated, they kind of deserve to be there in my opinion.

the random and thread_rng functions are moved from the thread_rng module to the top-level module, to have a cleaner story for the rngs module. The separation meant I had to add ThreadRng::new(), but it is pub(crate) until #404 is settled. I am for exposing both b.t.w., I see little trouble in it.

@dhardy
Copy link
Member

dhardy commented May 6, 2018

It would be good to get more feedback on this (bearing in mind that the prng module will likely be removed soon anyway; see #431). A rough summary of my thoughts:

  • the "external" RNGs (os, jitter and entropy) could easily be moved to a new module
  • the wrappers ReadRng and ReseedingRng don't feel like they fit in the same module, but don't need to be exposed at the top level
  • StdRng and SmallRng are fine at the top level, though they're not out of place in rngs either

The above re-ordering is in my opinion workable but probably not optimal. I'm wondering about an alternative like:

  • keep StdRng and SmallRng at the top level
  • move OsRng, JitterRng and EntropyRng to a sub-module like extrng, or maybe entropy
  • consider moving the others; e.g. ReadRng and ReseedingRng could be in adaptor

@dhardy dhardy added E-question Participation: opinions wanted B-API Breakage: API P-high Priority: high labels May 7, 2018
@dhardy
Copy link
Member

dhardy commented May 8, 2018

@pitdicker still waiting to hear your thoughts on my alternative proposal ↑. Putting the three external RNGs into an "entropy" module sounds okay to me.

@pitdicker
Copy link
Contributor Author

Sorry, I didn't have much time yesterday. I still think roughly the same as in #423 (comment). It would loose the advantage of having everything in one module.

If you look at it one way the rngs all offer pretty different functionality, in another they all just implement RngCore.

I think it gives a nice symmetry with the distributions module. I see that also as a collection of not entirely similar functionality. There are the real probability distributions, Alphanumeric, the special Standard, and Uniform is a bit special too. But they all implement the Distribution trait.

I think you wrote that it would split rand in two (can't find the comment). That is kind of the idea I had in mind, one part that deals with generating random numbers, another to transform it to the right types and distributions.

@dhardy
Copy link
Member

dhardy commented May 8, 2018

Personally I don't think all these "RNGs" fit together. ReseedingRng and ReadRng possibly fit in a misc module; they're not things that will be used (directly) frequently. The mock module can just stay where it is (before this PR) in my opinion, though not very important. Not really sure about the std/small/thread RNGs; if anything those should probably be the ones to go in an rngs module.

As for distributions, you're right that there's a lot of different stuff lumped together in there; I don't currently have any ideas for tidying it up but it's not too bad as is.

@dhardy
Copy link
Member

dhardy commented May 9, 2018

I think we should take some inspiration from the std library:

  • some modules are large (io, mem, fmt, collections); some are small (any, i8 etc., result); this is not a defining characteristic and allows things to be organised by topic without deep hierarchies
  • some traits like Read and Iterator are implemented by types from many different modules; this is not a bad thing
  • the top-level documentation points out some notable features but doesn't try to mention everything

I'll have a go at re-organising. I think we can still use some of the new doc you wrote 😄

@pitdicker
Copy link
Contributor Author

Closing in favour of #435.

@pitdicker pitdicker closed this May 10, 2018
@pitdicker pitdicker deleted the rngs_module branch June 8, 2018 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-API Breakage: API E-question Participation: opinions wanted P-high Priority: high
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants