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

StepRng #268

Merged
merged 2 commits into from
Feb 26, 2018
Merged

StepRng #268

merged 2 commits into from
Feb 26, 2018

Conversation

dhardy
Copy link
Member

@dhardy dhardy commented Feb 23, 2018

Port MockAddRng from my branch, but without SeedingRng impl and slightly different tests

I'm not quite sure on the name or whether it should be seedable, but amazingly this saves 5 independent implementations of RngCore in tests.

@pitdicker
Copy link
Contributor

Great! Surprising the resulting line count is only 10 less.

What do you think about moving it inside the test module in lib.rs?

@clarfonthey
Copy link
Contributor

Why not just StepRng?

@dhardy
Copy link
Member Author

dhardy commented Feb 24, 2018

It could be moved to the test module for internal use only, but it may well be useful to other users too.

@clarcharr the first time I tried introducing this I got criticized for not making it very clear from the name that it was not a real random number generator, though I guess mock::StepRng or test::StepRng should be good enough.

@pitdicker pitdicker mentioned this pull request Feb 24, 2018
33 tasks
@clarfonthey
Copy link
Contributor

@dhardy Honestly, I think that the docs are plenty, and that the name is unwieldy otherwise.

@dhardy
Copy link
Member Author

dhardy commented Feb 25, 2018

Renamed as suggested.

@pitdicker about 30 of the new lines are documentation. If put directly in the test module it would drop that. But is it not useful enough to be worth exporting? I wonder if we should have a rand-mock crate in the future — but perhaps better not to have too many tiny crates.

Edit: rebased to clean up and fix doc

@pitdicker
Copy link
Contributor

O, the number of lines was no critique, just an observation 😄.

But is it not useful enough to be worth exporting?

I don't know. It could be useful for some tests. But you would have to know quite some details of the implementation to use it when also using ranges, floats, distributions etc.

Crazy idea: what do you think about making it conditionally available with cfg(debug_assertions)? Then it will not accidentally end up in the code in release builds.

@dhardy
Copy link
Member Author

dhardy commented Feb 26, 2018

I've been thinking about putting more stuff behind feature flags. We could have a "mock" feature flag then put this behind #[cfg(any(test, features="mock"))].

But, true, this is mostly useful for testing distribution implementations, and many of them are in rand already, so maybe better not to export.

Edit: I realise there's not much point using a feature gate since the linker removes any unused components anyway; besides it complicates documentation. I don't see a "guard" against use in release builds as helping much anyway.

@dhardy dhardy changed the title MockAddRng StepRng Feb 26, 2018
@dhardy dhardy merged commit 893b50b into rust-random:master Feb 26, 2018
@dhardy dhardy deleted the mock branch March 14, 2018 16:40
pitdicker pushed a commit that referenced this pull request Apr 4, 2018
@dhardy dhardy mentioned this pull request Sep 23, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants