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

feat: Add a fork() method #49

Merged
merged 2 commits into from
Feb 17, 2023
Merged

feat: Add a fork() method #49

merged 2 commits into from
Feb 17, 2023

Conversation

notgull
Copy link
Member

@notgull notgull commented Feb 13, 2023

Closes #36 by adding a fork() method that can be used to clone the RNG with a new seed. This strategy is also used in Rng::new() to fork from the global RNG.

RNG.try_with(|rng| {
let current = rng.replace(Rng(0));

let mut restore = RestoreOnDrop { rng, current };
Copy link
Member

Choose a reason for hiding this comment

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

can we be sure that this doesn't corrupt the Rng in case of a panic? or doesn't that matter because the u64 seed doesn't have invalid states and thus we can just do that anyways? why isn't that an exposed functionality/method of Cell?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think this is an issue, since with &mut there shouldn't be any way to corrupt it with uninitialized data.

Copy link
Member

@fogti fogti Feb 17, 2023

Choose a reason for hiding this comment

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

ok, what is the function of the RestoreOnDrop here, why is it necessary? Or better, how is it supposed to work, because currently, the interaction seems to be a bit unclear to me. This isn't actually a blocker to this PR, but I would appreciate better documentation for this abstraction combo.

Copy link
Member Author

Choose a reason for hiding this comment

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

In order to use the Rng stored in the thread-local variable, the Rng needs to be replaced with a zero value and moved out, then moved back in once the operation is done. The RestoreOnDrop guard ensures that it's moved back in even in the event of a panic (which can happen if the range bounds are invalid), preventing the thread-local variable from being set to 0 and causing a loss of entropy.

Copy link
Member

Choose a reason for hiding this comment

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

I'd appreciate it if we would simply define the Rng to be Clone + Copy, which would allow us to use more convenient methods on Cell<Rng>.

@notgull notgull requested a review from fogti February 17, 2023 03:20
@notgull notgull merged commit 61adc59 into master Feb 17, 2023
@notgull notgull deleted the notgull/fork branch February 17, 2023 23:01
@notgull notgull mentioned this pull request Apr 7, 2023
notgull added a commit that referenced this pull request Jun 9, 2023
- **Breaking:** Remove interior mutability from `Rng`. (#47)
- Add a `fork()` method. (#49)
- Add a `no_std` mode. (#50)
- Add an iterator selection function. (#51)
- Add a `choose_multiple()` function for sampling several elements from an iterator. (#55)
- Use the `getrandom` crate for seeding on WebAssembly targets if the `js` feature is enabled. (#60)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

clone() does not behave as expected
2 participants