Skip to content

Improve doc on implementing Default and Clone#383

Merged
dhardy merged 5 commits into
rust-random:masterfrom
pitdicker:impl_default_doc
Apr 16, 2018
Merged

Improve doc on implementing Default and Clone#383
dhardy merged 5 commits into
rust-random:masterfrom
pitdicker:impl_default_doc

Conversation

@pitdicker

@pitdicker pitdicker commented Apr 10, 2018

Copy link
Copy Markdown
Contributor

Fixes #378.

cc @newpavlov

@newpavlov

newpavlov commented Apr 10, 2018

Copy link
Copy Markdown
Member

Looks good to me!

vks
vks approved these changes Apr 10, 2018
View reviewed changes

@vks vks left a comment •
edited
Loading

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.

Looks good to me as well!

@pitdicker pitdicker mentioned this pull request Apr 10, 2018
Merged
dhardy
dhardy reviewed Apr 11, 2018
View reviewed changes
Comment thread rand_core/src/lib.rs Outdated
/// support optional at the crate level in PRNG libs.
/// - `Clone`, but only if the clone will have identical output to the original
/// (i.e. be a true clone), or if the clone would be another handle to the
/// same generator.

@dhardy dhardy Apr 11, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I never really got the point of saying this. If you put anything in an Rc<..> type then clone() will copy the reference, not the object — so why even say it?

If you're going to be pedantic like this then you might as well update the next point about Copy too, though I don't see much point.

@pitdicker pitdicker Apr 12, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I copied it from your comment, but now understand why you left it out. With OsRng in mind I think it starts to make more sense, as that is the same principle but without Rc<..>?

@dhardy dhardy Apr 12, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sort of. I'm wondering if we should say much about Clone at all now. Maybe just that on normal PRNGs clone should imply identical output? Even that may not make sense given that we might try to make StdRng reseed on clone in the future(?).

@pitdicker pitdicker Apr 12, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I just reworded it as "Clone if possible"

@pitdicker

pitdicker commented Apr 12, 2018

Copy link
Copy Markdown
Contributor Author

Spelling out my concerns on implementing Clone for JitterRng in #384 (comment) helped me seeing it a bit more clearly, and I now believe it to not really be a problem (see the commit).

Even that may not make sense given that we might try to make StdRng reseed on clone in the future(?).

I think you mean ReseedingRng (StdRng does not do reseeding)? Did we discuss that before, as it is something I eventually would like to see? (No success yet implementing that, and it would also require EntropyRng to implement clone in practice)

@dhardy

dhardy commented Apr 12, 2018

Copy link
Copy Markdown
Member

Yes, I meant ReseedingRng. Sorry, forgetting that StdRng doesn't do the reseeding; I wonder if there is any point in it not doing so given that it's not supposed to be reproducible anyway and there's negligible aggregate performance difference?

I don't see any reason ReseedingRng can't reseed on clone?

@pitdicker

pitdicker commented Apr 12, 2018

Copy link
Copy Markdown
Contributor Author

Added a commit to implement Clone for ReseedingRng. It needs some more work (test+documentation), but do you think it is correct?

@pitdicker pitdicker changed the title Improve doc on implementing Default Improve doc on implementing Default and Clone Apr 12, 2018
@vks

vks commented Apr 12, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

Does it make sense to implement Clone when it is equivalent to just generating a new one?

@pitdicker

pitdicker commented Apr 12, 2018

Copy link
Copy Markdown
Contributor Author

Does it make sense to implement Clone when it is equivalent to just generating a new one?

I don't care much about the Clone implementations to be honest. But I suppose it helps with generic code, even when it sometimes basically means reinitializing an RNG.

@pitdicker pitdicker mentioned this pull request Apr 13, 2018
Merged
dhardy
dhardy reviewed Apr 13, 2018
View reviewed changes
Comment thread src/jitter.rs Outdated
/// [Jitterentropy](http://www.chronox.de/jent.html) version 2.1.0.
///
/// [`OsRng`]: ../os/struct.OsRng.html
#[derive(Clone)]

@dhardy dhardy Apr 13, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Don't we at the very least need to clear data_half_used to prevent next_u32 returning the same result from each part?

Otherwise we could simply implement this with JitterRng::new — except that timer might not be the default function.

@pitdicker pitdicker Apr 13, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Glad you are sharp 😄.

Comment thread src/reseeding.rs
inner: self.inner.clone(),
reseeder: self.reseeder.clone(),
threshold: self.threshold,
bytes_until_reseed: 0, // reseed clone on first use

@dhardy dhardy Apr 13, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice way of forcing a reseed!

@pitdicker pitdicker force-pushed the impl_default_doc branch 2 times, most recently from 6394566 to a68ec1d Compare April 13, 2018 16:26
dhardy
dhardy approved these changes Apr 15, 2018
View reviewed changes
Comment thread src/jitter.rs
data: self.data,
rounds: self.rounds,
timer: self.timer,
mem_prev_index: self.mem_prev_index,

@dhardy dhardy Apr 15, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do you think mem_prev_index should be adjusted?

@pitdicker pitdicker Apr 15, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think we are okay leaving it as is. They probably have different locations for the memory slice on the stack. Having the same location twice should not make the CPU much better at predicting. And after the first round the values will diverge, as memaccess is called a variable number of times.

@dhardy

dhardy commented Apr 16, 2018

Copy link
Copy Markdown
Member

Okay, I was going to merge, but there are conflicts.

@pitdicker

pitdicker commented Apr 16, 2018

Copy link
Copy Markdown
Contributor Author

Rebased

@dhardy dhardy merged commit 7b14992 into rust-random:master Apr 16, 2018
@pitdicker pitdicker deleted the impl_default_doc branch April 29, 2018 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vks vks vks approved these changes
@dhardy dhardy dhardy approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@pitdicker @newpavlov @dhardy @vks