Add new function gcd_lcm for calculating the gcd and lcm simultaneously (performance)#12
Add new function gcd_lcm for calculating the gcd and lcm simultaneously (performance)#12haudan wants to merge 1 commit intorust-num:masterfrom
Conversation
|
Checks failed for Rust 1.20 and below, most likely due to the use of the |
cuviper
left a comment
There was a problem hiding this comment.
Thanks, I like this idea, but we'll need a default implementation to avoid a breaking change.
| /// assert_eq!(10.gcd_lcm(&4), (2, 20)); | ||
| /// assert_eq!(8.gcd_lcm(&9), (1, 72)); | ||
| /// ~~~ | ||
| fn gcd_lcm(&self, other: &Self) -> (Self, Self); |
There was a problem hiding this comment.
Note that it's a breaking change to add a trait method without a default implementation. For instance, even num-bigint would fail to build with this until it adds the new method.
I think it should be straightforward to implement this generically, although it probably needs where Self: Clone to do the math by-value. When we do implement this for num-bigint, we can do so without those external clones, so overall this should be fine.
| #[test] | ||
| fn test_gcd_lcm() { | ||
| for i in 1..=255 { | ||
| for j in 0..=255 { |
There was a problem hiding this comment.
I think it will be fine to use a plain range for compatibility.
Maybe I'm missing something, but I'm surprised that this didn't warn about 255 being out of range for i8. But anyway, if you want to be exhaustive, perhaps we should use negative values too with -128..127.
I also notice that you avoid lcm(0, 0), but I think this is actually a bug in our existing implementation. We would get a divide-by-zero since gcd(0, 0) = 0, but Wolfram Alpha says lcm(0, 0) = 0 too.
|
#19 also added |
Code relying on
Integer::gcdandInteger::lcmfor identitcal inputs could be sped up by eliminating unecessary recomputations of the gcd. A function computing and returning both could eliminate implicitgcdcalls behind the scenes.