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

Require SeedableRng::Seed to impl Clone #1491

Merged
merged 3 commits into from
Sep 9, 2024

Conversation

clarfonthey
Copy link
Contributor

@clarfonthey clarfonthey commented Aug 30, 2024

  • Added a CHANGELOG.md entry

Summary

Adds a Clone bound to SeedableRng::Seed.

Motivation

Simply put: it's already possible to clone these seeds based upon the bounds already provided, but it's really annoying without a proper Clone bound. You can just as easily create a new one with Default and write all of the data from the old seed into the new one with AsMut, but this now requires me to do this explicitly instead of just being able to derive(Clone) on a struct that contains a seed.

Details

This is also a breaking change, but I don't think people will have a problem including it in the next breaking release.

@clarfonthey
Copy link
Contributor Author

Actually considering adding AsRef to this too, since I had no idea AsMut didn't imply it. It's more work for implementors, but is it really?

Honestly, I kinda wish that the seed parameter were just N to indicate the number of bytes needed, and we forced it to just be an appropriately sized array. But I dunno how people feel about that.

@dhardy
Copy link
Member

dhardy commented Sep 2, 2024

Honestly, I kinda wish that the seed parameter were just N to indicate the number of bytes needed, and we forced it to just be an appropriately sized array. But I dunno how people feel about that.

This design decision comes from a while back... but it appears that we still need an incomplete nightly-only feature to do this:

#![feature(generic_const_exprs)]

pub trait Seedable {
    const BYTES: usize;
    fn from_seed(seed: [u8; Self::BYTES]) -> Self;
}

struct MyRng;
impl Seedable for MyRng {
    const BYTES: usize = 12;
    fn from_seed(seed: [u8; 12]) -> Self {
        todo!()
    }
}

@clarfonthey
Copy link
Contributor Author

Yeah, the best workaround would be to use something like generic-array, which honestly seems preferable here.

@dhardy
Copy link
Member

dhardy commented Sep 6, 2024

I don't think we want to use generic-array — we've had plenty enough people complain about rand being too complicated or having too many dependencies. But once we get equivalent functionality in stable Rust we should consider revisiting this.

In the mean-time, please do add an AsRef bound. It will affect Seed512 but that's probably it.

@clarfonthey
Copy link
Contributor Author

Finally got to this, but now AsRef is in the list.

I updated the description slightly since Default is the only trait that isn't implemented for [T; N], although you will have to manually implement AsRef and AsMut since it isn't derivable.

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.

Thanks!

@dhardy
Copy link
Member

dhardy commented Sep 9, 2024

autoimpl supports AsRef and AsMut. But this is fine.

@dhardy dhardy merged commit 71c53be into rust-random:master Sep 9, 2024
14 checks passed
@clarfonthey clarfonthey deleted the clone-seed branch November 9, 2024 19:05
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