Docs for implementing SeedableRng for large seeds#381
Docs for implementing SeedableRng for large seeds#381pitdicker merged 5 commits intorust-random:masterfrom
SeedableRng for large seeds#381Conversation
|
The failure on nightly seems unrelated. |
| /// pub struct MyRngSeed(pub [u8; N]); | ||
| /// | ||
| /// impl Default for MyRngSeed { ... } | ||
| /// impl AsMut<u8> for MyRngSeed { ... } |
There was a problem hiding this comment.
With just a few more lines the example can be made complete. Maybe it is worth it to do so? (and then also does not need to be ignored)
There was a problem hiding this comment.
I expanded the example to actually compile. What do you think?
There was a problem hiding this comment.
Can you use fn default() -> MyRngSeed { MyRngSeed([0u8; 64]) } instead?
| /// for example `0xBAD5EEDu32` or `0x0DDB1A5E5BAD5EEDu64` ("odd biases? bad | ||
| /// seed"). This is assuming only a small number of values must be rejected. | ||
| /// Alternatively, the newtype pattern can be used to make sure only valid | ||
| /// seeds can be constructed: |
There was a problem hiding this comment.
I don't really like this as a pattern. RNG implementations can do as they wish, but we shouldn't advertise it here i.m.o.
It is useful for generic code to have from_seed work with all simple slices. The default implementation of from_rngrelies on this, and something like from_hashablewould too.
Also, does it really work? With the AsMut implementation wouldn't it be possible to modify the newtype outside new()?
There was a problem hiding this comment.
Also, does it really work? With the AsMut implementation wouldn't it be possible to modify the newtype outside new()?
Good point! I removed the suggestion to use the newtype pattern to work around invalid seeds.
This does not work, because the seed has to implement `AsMut<[u8]>`, so any invariants enforced by the constructor can be circumvented.
| /// type Seed = MyRngSeed; | ||
| /// | ||
| /// fn from_seed(seed: MyRngSeed) -> MyRng { | ||
| /// MyRng(MyRngSeed) |
There was a problem hiding this comment.
Should be MyRng(seed) (reason of the CI failure)
|
Looks good to me! Let's wait until @dhardy had a chance to look over it to before merging. |
|
Yep, looks good! 👍 @pitdicker can you sort out the merging? |
|
I restarted the Travis build. @pitdicker if you have a Travis account I think you should be able to do that too. |
|
I already restarted travis, but that was not enough |
3 participants
Fixes #354.