Add rand-core sub-crate and update version numbers#288
Add rand-core sub-crate and update version numbers#288dhardy merged 4 commits intorust-random:masterfrom
Conversation
|
Looks good to me. |
pitdicker
left a comment
pitdicker
left a comment
There was a problem hiding this comment.
Looks good to me! Left a few small comments.
rand-core/README.md
Outdated
| Core functionality for the [rand] library. | ||
|
|
||
| This crate contains the core trait, `RngCore`, and some extension traits. This | ||
| should be sufficient for libraries publishing an `RngCore` implementation, |
There was a problem hiding this comment.
This reads a like a suggestion, but we prefer RNG implementations to only depend on this crate, right? Can you reword a little?
| `rand` is primarily distributed under the terms of both the MIT | ||
| license and the Apache License (Version 2.0). | ||
|
|
||
| See LICENSE-APACHE, and LICENSE-MIT for details. |
There was a problem hiding this comment.
Should the license files (and the changelog) also be copied to the rand-core directory?
There was a problem hiding this comment.
I don't know, but that appears necessary to get the licence files inside the Cargo package, so I will add them.
rand-core/src/lib.rs
Outdated
| //! | ||
| //! The `impls` and `le` sub-modules include a few small functions to assist | ||
| //! implementation of `RngCore`. Since this module is only of interest to | ||
| //! `RngCore` implementors, it is not re-exported from `rand`. |
rand-core/src/lib.rs
Outdated
| /// | ||
| /// This trait encapsulates the low-level functionality common to all | ||
| /// generators, and is the "back end", to be implemented by generators. | ||
| /// End users should normally use [`Rng`] instead. |
There was a problem hiding this comment.
This sounds like an old comment. Maybe point to the rand crate now, instead of only the Rng trait. (also in the rest of this comment block)
rand-core/src/lib.rs
Outdated
| /// This trait encapsulates the low-level functionality common to all | ||
| /// pseudo-random number generators (PRNGs, or algorithmic generators). | ||
| /// | ||
| /// Normally users should use the [`NewRng`] extension trait, excepting when a |
| /// | ||
| /// Seeding a small PRNG from another small PRNG is possible, but | ||
| /// something to be careful with. An extreme example of how this can go | ||
| /// wrong is seeding an [`XorShiftRng`] from another [`XorShiftRng`], which |
There was a problem hiding this comment.
Do we still want this link to XorShiftRng? I removed it in the original PR because of the crate separation.
There was a problem hiding this comment.
I decided to keep this paragraph as is since it provides a clear warning and only mentions Xorshift as an example (we could even remove the link and it would still be clear).
This does leave a couple of cross-crate doc links, but I don't see a major problem (as long as we keep the number small).
|
Is now the right time to bring up if we should remove the default implementations in b.t.w. I upgraded my |
|
Agreed, default implementations is an issue to discuss. I'd like to try implementing some type of Okay, I'll go over the documentation again. |
This moves core traits/types/impls to rand-core impls and le modules are now public (from rand-core only) CI tweaks, needed since not all features are duplicated on rand-core Cross-crate doc links now use full URLs (is there a better option?)
|
Updated. Are you happy with this @pitdicker? The update adds two unrelated changes which might want extra attention:
|
|
Yes I am happy with it 😄, and very nice documentation. Ready to merge? |
|
Good! But no, I'm hoping to get a little more feedback on #291 first. |
|
Rebased without change from #291. |
Add rand-core sub-crate and update version numbers
3 participants
Uh oh!
There was an error while loading. Please reload this page.