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

Implement Rng for Box with alloc feature #227

Merged
merged 8 commits into from
Jan 11, 2018

Conversation

pitdicker
Copy link
Contributor

As discussed in #225 (comment).

I thought to run cargo test --no-default-features --feature=alloc, but that did not work. Endless compiler warnings.

I have included a couple a couple of commits that fix some of the tests that depend on needless memory allocations. Still about 40 compiler errors to go, most of which have to do with tread_rng not being available in lib.rs.

The test in lib.rs are where most of the remain problems are, and I didn't feel like fixing them at the moment. Those tests date from 5/6 years ago when rand.rs was the only file and things were very different. Several are now part of submodules, or could use a close look.

So testing on no_std does not work yet, but I hoped to leave that for another PR.

Copy link
Member

@dhardy dhardy left a comment

Choose a reason for hiding this comment

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

I was a bit shocked by the size of this PR, but on closer look I like what you've done!

src/lib.rs Outdated
@@ -625,7 +625,9 @@ impl<'a, R: ?Sized> Rng for &'a mut R where R: Rng {
}
}

#[cfg(feature="std")]
#[cfg(all(feature="alloc", not(feature="std")))]
use alloc::boxed::Box;
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 prefer if you put the use statement above, near #[cfg(feature="std")] use std::rc::Rc;.

src/reseeding.rs Outdated
// Sanity test: if we've gotten here, `fill_bytes` has not infinitely
// recursed.
assert_eq!(v.len(), FILL_BYTES_V_LEN);

// To test that `fill_bytes` actually did something, check that the
// average of `v` is not 0.
let mut sum = 0.0;
Copy link
Member

Choose a reason for hiding this comment

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

If you're going to tidy up, might as well just compare the sum here instead of average.

@dhardy
Copy link
Member

dhardy commented Jan 9, 2018

Oh, yes, but is the test framework even available? I was wondering about cargo test --no-default-features but IIRC the unit test framework itself isn't even available. (Try commenting out all test code.)

@pitdicker
Copy link
Contributor Author

That was my thought also, but I turns out to work. With everything commented out the PRNG tests run successfully.

@pitdicker
Copy link
Contributor Author

pitdicker commented Jan 10, 2018

I have replaced all uses of thread_rng() and ::test::rng() with ::test::rng(seed). I don't think it will win a beauty prize, but once we have better seeding and from_hashable we can make things better.

That took care of just about all errors left. Testing for no_std can be done with:

cargo test --tests --no-default-features
cargo test --tests --no-default-features --features=alloc

I did not replace thread_rng in the doctests. I don't want to use fixed seeds in the documentation, that would give the wrong impression. So doctests can only run with the std feature.

@pitdicker
Copy link
Contributor Author

(I rebased the first 5 commits, I believe a documentation change in chacha made it unmergable. But otherwise didn't touch them)

@dhardy
Copy link
Member

dhardy commented Jan 11, 2018

Nice work!

I agree, I don't think it's worth changing the doc-tests.

One final thing to do: add those new tests to CI.

@dhardy dhardy merged commit c2060ef into rust-random:master Jan 11, 2018
dhardy added a commit to dhardy/rand that referenced this pull request Jan 11, 2018
dhardy added a commit to dhardy/rand that referenced this pull request Jan 11, 2018
@pitdicker pitdicker deleted the no_std_alloc branch January 12, 2018 05:58
pitdicker pushed a commit that referenced this pull request Apr 4, 2018
 Implement Rng for Box with alloc feature
pitdicker pushed a commit that referenced this pull request Apr 4, 2018
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.

2 participants