Check for overflow in gcd algorithm #15228
Conversation
|
This is related to #15225 (but doesn't solve the issue). |
| u = abs(a >> za) | ||
| v = abs(b >> zb) | ||
| u = unsigned(abs(a >> za)) | ||
| v = unsigned(abs(b >> zb)) |
There was a problem hiding this comment.
would checked_abs give the same exception here as the general T<:Integer version?
There was a problem hiding this comment.
checked_abs is too strict here; if a == b == typemin(Int), then unsigned(abs(...)) is well-defined, but checked_abs would fail.
I just see that this code right-shifts before calling abs, and given that typemin(Int) has many trailing zeros, the code is probably fine.
No, they give different errors. checked_abs returns OverflowError, whereas type conversions return InexactError. (I find that weird, but that's how things are.) The generic gcd algorithm also might return DivideError instead.
|
The tests fail because of |
eschnett
force-pushed
the
eschnett/gcd
branch
from
fb3baad
to
f44b1b1
Compare
|
The 64-bit Appveyor tests timed out; there wasn't an actual failure reported. |
|
And since Int32 and Int64 always go to different algorithms, it would be beneficial for test coverage to always test this for both integer sizes. |
|
Now always throws |
|
👍 |
| @@ -54,7 +57,7 @@ function gcdx{T<:Integer}(a::T, b::T) | |||
| s0, s1 = s1, s0 - q*s1 | |||
| t0, t1 = t1, t0 - q*t1 | |||
| end | |||
| a < 0 ? (-a, -s0, -t0) : (a, s0, t0) | |||
| a < 0 ? (checked_neg(a), checked_neg(s0), checked_neg(t0)) : (a, s0, t0) | |||
eschnett
force-pushed
the
eschnett/gcd
branch
2 times, most recently
from
29a4cde
to
4837b86
Compare
|
could use a squash to get rid of the whitespace failures at intermediate commits |
eschnett
force-pushed
the
eschnett/gcd
branch
2 times, most recently
from
643f7a9
to
3e8388b
Compare
The gcd and lcm algorithms are somewhat expensive (requiring a loop and shifts or divisions), so an overflow check doesn't hurt performance much.
eschnett
force-pushed
the
eschnett/gcd
branch
from
3e8388b
to
5e20b6c
Compare
|
Rebased and squashed, waiting for Appveyor and Travis |
Check for overflow in gcd algorithm
The gcd algorithm is somewhat expensive (requiring a loop and shifts or divisions), so an overflow check doesn't hurt performance much.