Add LCM for Uint #1028
Add LCM for Uint #1028
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1028 +/- ##
==========================================
- Coverage 79.87% 79.83% -0.04%
==========================================
Files 163 164 +1
Lines 17737 17801 +64
==========================================
+ Hits 14167 14212 +45
- Misses 3570 3589 +19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| assert_eq!(lhs.lcm_uint(&rhs), target); | ||
| } | ||
|
|
||
| fn run_tests<const LIMBS: usize, const WIDE_LIMBS: usize>() |
There was a problem hiding this comment.
What's our policy on proptests? Seems like it would be useful to have one here.
There was a problem hiding this comment.
I'm definitely a fan of proptests
There was a problem hiding this comment.
By proptest, do you mean this proptest? I have never used it before, so what kind of tests would you like here? Generate 2 primes (it seems difficult to get it to generate primes) and calculate that the LCM output equals to multiple of the inputs? Or would it be used only for detecting whether the function crashes with any kind of input?
There was a problem hiding this comment.
I think there are two types of tests possible here. First, with primes; but you don't need filtering for those, just use random bytes as a seed for a random prime. Second, edge cases, for which you don't need primes, just random numbers. num_bigint's lcm can be used as the reference.
There was a problem hiding this comment.
@Fethbita you can look here for some example protests: https://github.com/RustCrypto/crypto-bigint/blob/master/tests/uint.rs
I guess I would be okay with not having them for the initial PR, but it would definitely be good to have them as a followup, whoever authors it. It's something I can look at.
Truly random primes seem like they might need a circular dependency on crypto-primes which is doable (it can be added as a dev-dependency, I think) but a little tricky.
Another option is to write the test with a canned set of primes and select from them randomly, though if the solution space is small enough it could just be brute forced every test run and kind of defeats the point of proptest.
There was a problem hiding this comment.
I added a single test for comparing with num_bigint, using random values.
src/uint/lcm.rs
Outdated
|
|
||
| impl<const LIMBS: usize> Uint<LIMBS> { | ||
| /// Compute the least common multiple of `self` and `rhs`. | ||
| pub const fn lcm_uint<const WIDE_LIMBS: usize>(&self, rhs: &Self) -> Uint<WIDE_LIMBS> |
There was a problem hiding this comment.
I'm not sure I like this _uint naming scheme, which I guess was introduced in #923
It feels to me like _uint should be the "default" for Self/Uint, so this is just fn lcm
There was a problem hiding this comment.
Right, removed the _uint.
There was a problem hiding this comment.
It's kinda the same issue here as in the other PR. If there's an LCM trait, then the inherent method can clobber the trait method, making combinations like Uint/Int more difficult to use in non-generic contexts.
There was a problem hiding this comment.
@andrewwhitehead that's fine so long as they have the same type signature. You can always call the inherent method with UFCS.
If they have different type signatures, that's bad.
4 participants
I only added the LCM for
Uintfor now, I think it should not be difficult to add it to other types as well. I have never used the Int type so if that is different, might take some time.