Skip to content

Conversation

@dhardy
Copy link
Member

@dhardy dhardy commented Dec 6, 2025

  • Added a CHANGELOG.md entry

Summary

This is just a cut-down version of the block code: we don't need most of it.

Motivation

I think this hits most of your goals outside of le / utils, @newpavlov? No rewrite required.

Details

We don't need BlockRng64, so long as we're okay with abandoning the half-sample code (next_u32). I think that's fine.

We don't need to implement RngCore / CryptoRng on BlockRng either.

It turns out that we do need to keep CryptoGenerator if we want to allow ReseedingRng to implement CryptoRng in a generic fashion. I suppose otherwise we could implement CryptoRng only on ThreadRng (which we already do).

We also need to implement SeedableRng on the core, otherwise ReseedingRng doesn't work.

Unsolved here: fill_bytes is generic over W: Observable which is a private trait (essentially @newpavlov's Word trait with less features). So I think it's either that trait or fill_bytes_from_u32 and fill_bytes_from_u64 which isn't exactly neat design.

@dhardy dhardy requested a review from newpavlov December 6, 2025 07:48
@dhardy
Copy link
Member Author

dhardy commented Dec 6, 2025

Rand compatibility: rust-random/rand#1690

@dhardy
Copy link
Member Author

dhardy commented Dec 6, 2025

List of changes:

  • Remove outdated documentation.
  • Improve doc example in block
  • Make BlockRng functionality generic; most functionality requires only W: Clone though next_u64_from_u32 requires W = u32 and fill_bytes requires W: Word.
  • Do not implement RngCore on BlockRng; use inherent methods instead. This is required by the above since what was next_u32 now returns W and since next_u64_from_u32 has an additional (effective) W = u32 bound.
  • Do not implement CryptoRng on BlockRng (required by above).
  • Do not implement SeedableRng for BlockRng. Motivation: this is never used since BlockRng is always wrapped.
  • Remove BlockRng64 code. Only it's next_u32 method differs in functionality from what is now available on BlockRng, and I believe this is an acceptable loss in functionality. It is not implementable on BlockRng since it requires an additional half_used field (and changes to the behaviour of other methods).
  • Rename trait ObservableWord and make a public sealed trait.

Questions:

  • Should all BlockRng functionality have a W: Word bound? Either way should be fine, but std often prefers to use the least bounds possible.
  • Should we expand Word and replace le methods as in Replace block module with helper functions #24? If so, this would not be implemented in this PR.

Copy link
Member

@newpavlov newpavlov left a comment

Choose a reason for hiding this comment

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

As I wrote before, I don't think we need the Generator trait. I also really don't like the manual wrapping of BlockRng in implementation crates. It just results in a bunch of annoying boilerplate. Additionally, it's not clear how you intend to allow serialization/deserialization for Generator-based RNGs.

Finally, the separate index field is redundant and only results in bloating structure size of buffered RNGs.

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.

3 participants