Skip to content

Require SeedableRng::Seed to impl Clone#1491

Merged
dhardy merged 3 commits into
rust-random:masterfrom
clarfonthey:clone-seed
Sep 9, 2024
Merged

Require SeedableRng::Seed to impl Clone#1491
dhardy merged 3 commits into
rust-random:masterfrom
clarfonthey:clone-seed

Conversation

@clarfonthey

@clarfonthey clarfonthey commented Aug 30, 2024

Copy link
Copy Markdown
Contributor
  • 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
Copy Markdown
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

dhardy commented Sep 2, 2024

Copy link
Copy Markdown
Member

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
Copy Markdown
Contributor Author

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

@dhardy

dhardy commented Sep 6, 2024

Copy link
Copy Markdown
Member

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
Copy Markdown
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.

@dhardy dhardy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks!

@dhardy

dhardy commented Sep 9, 2024

Copy link
Copy Markdown
Member

autoimpl supports AsRef and AsMut. But this is fine.

@dhardy dhardy merged commit 71c53be into rust-random:master Sep 9, 2024
@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