Avoid overflow #42
Avoid overflow #42
Conversation
Tries to fix #13 by using least common multiple (LCM) for denominator when summing Ratios. i.e. 1/255 + 1/255 -> (1+1)/LCM(255,255) -> 2/255 instead of 1/255 + 1/255 -> (1*255 + 1*255)/(255*255), which would cause an overflow if the integer type is u8. Also adds minmal tests making sure 1/u8::max_value() + 1/u8::max_value() doesn't overflow for all integer types
|
This fails in rust 1.15.0 because |
|
Can I get some code review? maybe from @cuviper? |
|
Hey @cuviper it seems like you're actively maintaining this project. Could you please let me know how to get code review? |
There was a problem hiding this comment.
Hi, thanks for the PR, and sorry for the late review. It's a good approach, and I'm not too concerned with the overhead since Ratio does such gcd reductions frequently anyway.
I think we should also address Mul, Div, and the Assign ops before we call #13 fixed though.
add assertions that multiplying a number near the max value of its data type do indeed overflow. These assertions aim to guarantee that naive implementations of those operations would indeed overflow, in other words, that the generated large number is indeed large
|
So many failed builds! I learned my lesson. always run |
|
This PR now addresses |
|
Checked operations in this crate only happen between |
|
Pending further review, I believe this branch is ready for a merge |
|
Should |
|
In addition to all the extraneous |
|
Thanks! bors r+ |
42: Avoid overflow r=cuviper a=maxbla fix #13 this addresses the fact that 1/255 as `Rational<u8>` + 1/255 as `Rational<u8>` caused an overflow even though 2/255 is representable as a `Rational<u8>`. This overflow also occured for subtraction and modulo, as these are implemented with the same macro. This PR addresses the issue by calculating the LCM of denominators rather than naively multiplying denominators, then simplifying. Basically pre-simplify, then add, rather than add then simplify. As a comment reads, `Abstracts a/b op c/d = (a*lcm/b op c*lcm/d)/lcm where lcm = lcm(b,d)` (in this case op is exactly +,- and %) The performance penalties of this approach have not been measured, but I would expect it to be a moderate performance regression for word-sized integer types, as calculating LCM is O(logn) whereas the naive approach requires only multiplication and addition, O(1). For BigInts, I expect the performance regression to be minimal, as the old approach required multiplication, O(nlogn)? While calculating the LCM is also O(nlogn) for BigInts. Co-authored-by: Max Blachman <blachmanmax@gmail.com>
Build succeeded |
1352: [HOLD] fix: conduct GCD before rational ops r=doitian,keroro520 a=u2 updated: 1. rust-num/num-rational#42 2. For mul/div Rational or U256, we use new_raw is well, the same as add/sub U256, we assume that the Rational is already reduced when initialized. 3. for add/sub Rational, we compare the denom firstly. Co-authored-by: u2 <zhangyaning1985@gmail.com>
fix #13
this addresses the fact that 1/255 as
Rational<u8>+ 1/255 asRational<u8>caused an overflow even though 2/255 is representable as aRational<u8>. This overflow also occured for subtraction and modulo, as these are implemented with the same macro. This PR addresses the issue by calculating the LCM of denominators rather than naively multiplying denominators, then simplifying. Basically pre-simplify, then add, rather than add then simplify. As a comment reads,Abstracts a/b op c/d = (a*lcm/b op c*lcm/d)/lcm where lcm = lcm(b,d)(in this case op is exactly +,- and %)The performance penalties of this approach have not been measured, but I would expect it to be a moderate performance regression for word-sized integer types, as calculating LCM is O(logn) whereas the naive approach requires only multiplication and addition, O(1).
For BigInts, I expect the performance regression to be minimal, as the old approach required multiplication, O(nlogn)? While calculating the LCM is also O(nlogn) for BigInts.