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.

Sign up for GitHub

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

Jump to bottom

Add trait implementation guidance #312

Merged
merged 2 commits into from
Mar 20, 2018
Merged

Add trait implementation guidance #312

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e1244a6
Add trait implementation guidance
dhardy Mar 18, 2018
5dc1af7
Improve guidance on RngCore trait impls, in response to comments
dhardy Mar 19, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions rand-core/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,10 +83,15 @@ pub mod le;
///
/// It is recommended that implementations also implement:
///
/// - `Debug` but such that no internal details are output
/// - `Serialize` and `Deserialize` (from Serde), possibly behind a feature gate
/// - `Clone` if, and only if, the clone will have identical output to the original
/// - `Debug` with a custom implementation which *does not* print any internal
/// state (at least, `CryptoRng`s should not risk leaking state through Debug)
/// - `Serialize` and `Deserialize` (from Serde), preferably making Serde
/// support optional at the crate level in PRNG libs
/// - `Clone` if, and only if, the clone will have identical output to the
/// original (i.e. all deterministic PRNGs but not external generators)
/// - *never* implement `Copy` (accidental copies may cause repeated values)
/// - also *do not* implement `Default`, but instead implement `SeedableRng`
/// thus allowing use of `rand::NewRng` (which is automatically implemented)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not use NewRng for Default?

Copy link
Member Author

@dhardy dhardy Mar 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

PRNGs will be implemented in various crates outside of the rand crate, which provides NewRng. Default comes from the standard library. Therefore the locality of implementation rules make this impossible. PRNGs could implement Default themselves, but they'd have to depend on rand to do so, which I don't think they should. Other than that I guess it's a good idea, though possibly a little confusing.

/// - `Eq` and `PartialEq` could be implemented, but are probably not useful
///
/// # Example
Expand Down
3 changes: 2 additions & 1 deletion src/thread_rng.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@ use reseeding::ReseedingRng;
const THREAD_RNG_RESEED_THRESHOLD: u64 = 32*1024*1024; // 32 MiB

/// The type returned by [`thread_rng`], essentially just a reference to the
/// PRNG in thread-local memory.
/// PRNG in thread-local memory. Cloning this handle just produces a new
/// reference to the same thread-local generator.
///
/// [`thread_rng`]: fn.thread_rng.html
#[derive(Clone, Debug)]
Expand Down