Skip to content

SeedableRng::Seed and from_hashable - #62

Closed
dhardy wants to merge 10000 commits into
masterfrom
seedable
Closed

SeedableRng::Seed and from_hashable#62
dhardy wants to merge 10000 commits into
masterfrom
seedable

Conversation

@dhardy

@dhardy dhardy commented Nov 24, 2017

Copy link
Copy Markdown
Owner

Edit: PR no longer shows the code thanks to our rebasing of Rand history, but the commit is still available here.

Implement seeding via hash function, as discussed in #18.

This is a very rough implementation.

To discuss, regarding the hash function:

  • SeaHash is exposed for general usage, simply because it can be (if we're going to do this, we must stabilise it, so there's no worry that it will be replaced). This is a little odd, but seems sensible on the whole.
  • We could hide SeaHash and from_hashable behind a feature gate (hash or seahash) until we're more comfortable with the idea — that allows stabilising rand-core without this feature, in theory. But hopefully we don't need to do this.
  • More AsBytesFixed impls are needed, e.g. for u64
  • I'm unsure that AsBytesFixed is the best way of going about this. I'm considering moving the hashing into the trait implementation with trait Hash<H> { fn hash(state: &mut H, val: Self); } or similar. This allows specialised implementations of SeaHash.
  • How general do we make our SeaHash impl? I mean, do we allow pushing multiple items to it? This is functionality we don't need but requires very little extra code and possibly a small performance hit, but it does make the SeaHash impl we expose significantly more useful to other users. (We could also consider whether from_hashable should be more general, e.g. consume a SeaHash struct.)

Regarding SeedableRng:

  • Making Seed an associated type is a compromise; SeedSize associated constant doesn't appear to work; we do however restrict Seed to types supported by Finalize
  • Making Seed a u64 array instead of u8 array is certainly more convenient for us; any reason we shouldn't do this?
  • The new SeedableRng::from_seed impls for Isaac aren't 100% backwards compatible (old version allowed much larger seeds).

Lots to discuss, but I think this is moving forward. (@ticki if you get the time, I'd love your thoughts.)

Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.