Select "SimpleRand" over generic "Rand" for backwards compatibility #71
Conversation
src/distributions/uniform.rs
Outdated
| pub fn uniform<T: Rand<Uniform>, R: Rng+?Sized>(rng: &mut R) -> T { | ||
| T::rand(rng, Uniform) | ||
| pub fn uniform<T, R: Rng+?Sized>(rng: &mut R) -> T where Uniform: Distribution<T> { | ||
| Uniform.sample(rng) | ||
| } |
There was a problem hiding this comment.
Before looking at this PR I was just wondering what this function is for. It is not in Rand 0.3.*, right? It seems similar to the new distributions::Rand::rand but only works for integers?
There was a problem hiding this comment.
It's redundant... IIRC I added it before making the Uniform distribution. It should be removed.
|
Updated: I removed the |
| // Rejection sampling. About 0.2% of numbers with at most | ||
| // 21-bits are invalid codepoints (surrogates), so this | ||
| // will succeed first go almost every time. | ||
| match char::from_u32(rng.next_u32() & CHAR_MASK) { |
There was a problem hiding this comment.
This is now rejecting about half the results because char <= 0x10_ffff. There is probably a more optimal way, but I would not worry about it for this PR.
There was a problem hiding this comment.
Very weird. You're right, not something worth trying to solve here (if it even matters much: requiring almost 2 samples on average isn't that slow, and I doubt this distribution is going to get used much outside of "fuzz testing").
There was a problem hiding this comment.
rng.next_u32() / 0x11_0000 may do the trick. The division should get optimized to one multiply and shift-right.
There was a problem hiding this comment.
Trying to divide by the range again!
I would suggest we use your new range code, but who knows how long it'll be before CTFE is sophisticated enough.
| } | ||
| } | ||
| } | ||
|
|
||
| impl Distribution<char> for AsciiWordChar { |
There was a problem hiding this comment.
Was it you who made the comment that this should probably renamed? AlphanumericChar seems to better describe this. Does it make sense to also add a distribution with all printable ASCII characters?
There was a problem hiding this comment.
Possibly. Lets leave that for later #73
src/distributions/uniform.rs
Outdated
| } else { | ||
| i64::rand(rng, Uniform) as isize | ||
| Distribution::<i64>::sample(&Uniform, rng) as isize |
There was a problem hiding this comment.
Does it make sense to just use next_u32 and next_u64 here and for usize?
Maybe it is also better to change the conditions to test for 64-bit. See the open issue Policy for assumptions about the size of usize. But not a big deal.
if mem::size_of::<isize>() == 8 {
next_u64() as isize
} else {
// 32-bit or smaller
next_u32() as isize
}
This makes the new
Randbasically the same API as the oldRand, but defined viaDefault. This should make it a drop-in replacement, in theory.rand-derivewill need a bit of tweaking but should be relatively easy.