smol-rs / fastrand

A simple and fast random number generator
Apache License 2.0
386 stars 33 forks source link

matklad's dream fastrand #40

Open matklad opened 1 year ago

matklad commented 1 year ago

I love fastrand, it's the closest to the ideal prng crate for me, but it is annoyingly not quite there. As I've been thinking about this today, I decided to write my thoughts down. This is very explicitly not a proposal for a change -- I think there's just a difference in values between what fastrand is, and what I want from my idiosyncratic crate. Still, might be interesting for someone. Or maybe one day I'll fork more_opinionated_fastrand or something :)

The overall theme is "remove magic, it's ok if the user needs to write some code themselves if that leads to greater clarity".

So, the list of not-suggested changes:

#[derive(Debug, PartialEq, Eq)]
pub struct Rng { s: u64 }

impl Rng {
    pub fn new(seed: u64) -> Rng {
        Rng { s: seed }
    }
    #[cfg(feature = "std")]
    pub fn from_entropy() -> Rng {
        Rng::new(random_seed())
    }
    pub fn seed(&self) -> u64 {
        self.s
    }
}

/// get me a random without getrandom
#[cfg(feature = "std")]
fn random_seed() -> u64 {
    std::hash::Hasher::finish(&std::hash::BuildHasher::build_hasher(
        &std::collections::hash_map::RandomState::new(),
    ))
}
notgull commented 1 year ago

Hello!

Regarding your first and second points, having a version of fastrand that can be no_std and Sync has been on my to-do list for a while. I'm not a fan of the API changes; IMO we should value the case where we don't mind if a seed is generated for us over the other cases.

I agree with the magical Clone, see #38.

My general policy is that, if something can implement Default, it should implement Default. There are certain containers that assume this.

I'd be against removing the thread-local API. One of this crate's primary purposes is to serve as a global PRNG for some of our crates that need it, like futures-lite and async-executor (off the top of my head, there are probably others). I'd avoid pushing in the direction to make this harder. That being said, I wouldn't be opposed to moving this API to behind a feature flag.

I personally disagree with the separate u32_range function, as I don't really mind the .. at the call site.

I'd be for adding a bool function, not so sure about bool_ratio.

fogti commented 1 year ago

interior mutability and Clone are now fixed on master.