Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
CHANGELOG.mdentrySummary & motivation
It was noted in #13 that the
implrelationship betweenRngCoreandTryRngCoreis unsatisfactory. @newpavlov proposed a revision whereRngCore: TryRngCore. This PR goes a bit further, merging the two traits.RngCoreTryRngCore→TryRng. (Opinionated and not required. I also considered renaming it toRngCore.)trait InfallibleRng: TryRng<Error = Infallible>. This is effectively a trait alias with no methods of its own.CryptoRngandTryCryptoRngintoCryptoRng: TryRngResults (motivation):
InfallibleRngas a "trait" or "trait alias")impls on RNGs; in particular&mut R: TryRngforR: TryRngUnwrapMutis no longer needed(And yes, I can see the hypocrisy of suggesting these breaking changes after complaining about others, but preparing for
rand_corev1.0 means deciding whether to deal with existing issues or stabilise what we already have.)Optimality
In theory there should be little impact on the generated code, but it's clear the result isn't identical. I ran all
randbenchmarks againstmaster; those with at least 5% time change:The majority of results have no significant change and the above have roughly as many wins as losses, though there are a few more significant losses. TODO: look into this more.
Alternatives
UnwrapMutand cannot have&mut R: TryRngCoreforR: TryRngCore.RngCoreandTryRngCore(possible via macros).TryRngCore<Error = Infallible>(same as this PR).dyn RngCorevtables have extra methods.RngCore(orInfallibleRng) derivesTryRngCore<Error = Infallible> + Sized. This implies a need to usedyn TryRngCore<Error = Infallible>instead ofdyn InfallibleRng, but also that the vtable will not feature bothtry_next_u32andnext_u32.trait-aliasfeature we could actually makeInfallibleRnga trait alias. (I don't know whether switching to this this once stable would be a breaking change.)Details
See also:
rand_core::TryRnggetrandom#772rand_core::TryRngrand#1706Concerns
Would it be preferable to stabilise this code in
rand_corev1.0 over what we have now? I think so but would very much like feedback on this. CC @tarcieri @balooHow is usability of the result?
InfallibleRngrequires a tiny bit more boilerplate.InfallibleRngwithoutrand::Rngis a slight pain without the infallible methodsnext_u32()etc.; one must either.unwrap()(good for tests) or usematchto unpack theResult. In PRNG crates this is a small (IMO acceptable) burden.rand::Rngis basically unaffectedrand_coreusers, e.g. RustCrypto?How much work is involved in dealing with the changes?
RngCoreimplementations to useTryRngwithtype Error = Infallibleand different method prototypes is a minor PITA. This affects us more than users; updating the rust-random/rngs repo will be a pain.rand::Rngis basically unaffected, except that usages ofRngCoremust be replaced withRngorInfallibleRngrand_core::RngCoreinfallible methods (next_u32etc.) withoutrand::Rnghas to find some alternative