-
-
Notifications
You must be signed in to change notification settings - Fork 2
Merge RngCore, TryRngCore
#41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I don't think bigger vtables is an important factor, but I am fine with not having any additional methods on the infallible trait. It also would make it easier to migrate to the trait alias in future.
Also Rng would be a better and more consistent name than InfallibleRng in my opinion. It would follow the precedent introduced by std with its TryFrom/From and TryInto/Into traits. If you don't want to rename rand::Rng to RngExt, then we probably could have both rand::Rng and rand_core::Rng, the former being an extension trait for the latter. It could be somewhat confusing, but probably would work fine in practice.
| /// | ||
| /// This trait encapsulates the low-level functionality common to all | ||
| /// generators, and is the "back end", to be implemented by generators. | ||
| /// End users should normally use the [`rand::Rng`] trait |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's worth to keep this reference to rand::Rng in the trait docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't even make a "low effort" on the docs yet (until I have a better idea of whether we're doing this); some stuff will need un-deleting.
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