Rand core changes #419
Rand core changes #419
Conversation
|
Forgot about #376, will add that to this PR later today. |
rand_core/CHANGELOG.md
Outdated
| - Enable the `std` feature by default. (#409) | ||
| - Change `BlockRng64::inner` and add `BlockRng64::inner_mut` to mirror `BlockRng`. (#419) | ||
| - Add `BlockRng{64}::index` and `BlockRng{64}::generate_and_set`. (#374, #419) | ||
| - Add `AsMut<[Self::Item]>` bound to `BlockRngCore::Results` (future-proofing). (#419) |
There was a problem hiding this comment.
Since this is technically a breaking change, I think it would be better to rephrase as "Change BlockRngCore::Results bound to also require AsMut<[Self::Item]>".
And since we have two small breaking changes shall we call this 0.2.0 instead?
| /// Return a mutable reference the wrapped `BlockRngCore`. | ||
| pub fn inner(&mut self) -> &mut R { | ||
| pub fn inner_mut(&mut self) -> &mut R { |
There was a problem hiding this comment.
Since we must expose both inner and inner_mut and we will always have exactly one "core", there doesn't seem to be any advantage over just making core a pub field (which would be nicer syntactically). Since we have to make a breaking change here anyway (and probably no other users will be affected yet), shall we just do this?
For the other fields there is some advantage in keeping them private, but not really for core I think.
There was a problem hiding this comment.
I don't think it matters much one way or the other. It feels a little strange to me to have one public field, but the rest private.
There was a problem hiding this comment.
It works reasonably well IMO — typical usage is like ChaChaRng(BlockRng { core: ChaChaCore, .. }); i.e. the outer type is free to access the inner type however is deemed fit, whereas the other fields are not quite so straightforward.
| } | ||
|
|
||
| impl<R: BlockRngCore<Item=u64>> RngCore for BlockRng64<R> | ||
| where <R as BlockRngCore>::Results: AsRef<[u64]> | ||
| where <R as BlockRngCore>::Results: AsRef<[u64]> + AsMut<[u64]> |
There was a problem hiding this comment.
Do we actually need the new bound here? (Also for BlockRng above.)
There was a problem hiding this comment.
I initially tried to add the bound only on BlockRngCore, but it doesn't compile without the bound in a few more places.
pitdicker
force-pushed
the
rand_core_changes
branch
from
3625bc1
to
7167884
Compare
|
Is this PR ready to be merged? We can always make the field public later, right? |
pitdicker
force-pushed
the
rand_core_changes
branch
from
7167884
to
ecf5bcb
Compare
pitdicker
force-pushed
the
rand_core_changes
branch
from
ecf5bcb
to
3a8ae17
Compare
|
Yes, this is ready Sorry, seems to be fine. |
I forgot to change
BlockRng64::innerto return a reference and to addinner_mutin #325 (comment).This PR also copies the
indexandgenerate_and_setmethods from #374 toBlockRng64, and updates the changelog.Edit: the build will fail because I added
#![deny(missing_docs)]. The offending line is fixed in #374; as it gets fixed anyway it seemed nice to prevent a merge conflict.