Remove bit precision from the public API #46
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #46 +/- ##
=======================================
Coverage 99.32% 99.33%
=======================================
Files 9 9
Lines 1340 1347 +7
=======================================
+ Hits 1331 1338 +7
Misses 9 9 ☔ View full report in Codecov by Sentry. |
| @@ -8,29 +8,21 @@ use crate::{ | |||
|
|
|||
| /// Provides a generic way to access methods for random prime number generation | |||
| /// and primality checking, wrapping the standalone functions ([`is_prime_with_rng`] etc). | |||
| pub trait RandomPrimeWithRng { | |||
| pub trait RandomPrimeWithRng: Zeroize { | |||
There was a problem hiding this comment.
Why an additional Zeroize requirement?
| @@ -59,7 +69,7 @@ impl<T: Integer + RandomMod> MillerRabin<T> { | |||
| } | |||
|
|
|||
| /// Perform a Miller-Rabin check with a given base. | |||
| pub fn test(&self, base: &T) -> Primality { | |||
| pub fn test(&self, base: T) -> Primality { | |||
There was a problem hiding this comment.
Doesn't seem like taking base by value here reduces the amount of cloning.
| /// | ||
| /// For example, a U512 type occupies 8 64-bit words, but the number `7` contained in such a type | ||
| /// has a bit length of 3 because 7 is `b111`. | ||
| pub fn bits(&self) -> u32 { |
There was a problem hiding this comment.
Let's make it pub(crate) for now, since it's only used internally.
| @@ -9,25 +9,26 @@ use rand_core::CryptoRngCore; | |||
|
|
|||
| use crate::hazmat::precomputed::{SmallPrime, RECIPROCALS, SMALL_PRIMES}; | |||
|
|
|||
| /// Returns a random odd integer with given bit length | |||
| /// (that is, with both `0` and `bit_length-1` bits set). | |||
| /// Returns a random odd integer with given bit length (that is, with both `0` and `bit_length-1` | |||
There was a problem hiding this comment.
This is not going to affect merging this PR, but I just want to note that the original formatting was intentional - it's easier to read text that is 50-70 characters wide, so I try to add line breaks at meaningful points in the sentences.
| /// If the Lucas test is inconclusive, run a Miller-Rabin with random base and unless this second | ||
| /// M-R test finds it's composite, then conclude that it's prime. | ||
| fn _is_prime_with_rng<T: Integer + RandomMod>(rng: &mut impl CryptoRngCore, num: Odd<T>) -> bool { | ||
| let candidate = num.clone(); |
There was a problem hiding this comment.
Probably a little cleaner would be to call the argument candidate right in the method signature.
|
The original intent of the API was that someone who was using boxed uints could independently select the allocation size of the result. But I suppose given how many times we clone within the library already, and how much cloning will be going on later in most applications, one more reallocation to widen the result is not going to make much of a difference. |
An attempt to provide a more ergonomic API that works for both heap and stack allocated uints.
Before we had:
With this PR:
I think this is a more ergonomic public API.
Perhaps more contentiously, this PR also switches to prefer passing numeric parameters by value rather than by reference.
If reviewers think this makes the PR too noisy to review, I can split out those changes.