Check for overflow when negating typemin in Rational #31090
Conversation
|
last two updates removed trailing whitespaces |
|
I don't understand the Appveyor failure on x86_64 when it passes on i686 (and should care). My PR is to plug overflow on |
base/rational.jl
Outdated
| end | ||
| @noinline __throw_rational_argerror(T) = throw(ArgumentError("invalid rational: zero($T)//zero($T)")) | ||
| @noinline __throw_rational_ovferror(T) = throw(OverflowError("num[den] is typemin(T) and den[num] is negative")) |
There was a problem hiding this comment.
I find this error message a bit confusing: perhaps two different errors for the two cases? Or vaguer wording about one of the numerator or denominator being too large?
There was a problem hiding this comment.
Thanks for the note. Revised it:
@noinline __throw_rational_ovferror(num::T, den::T) where {T} =
(num < den) ? throw(OverflowError("typemin($T)//$den")) : throw(OverflowError("$num//typemin($T)"))
| @@ -19,7 +19,12 @@ using Test | |||
| @test -7//0 == -1//0 | |||
|
|
|||
| @test_throws OverflowError -(0x01//0x0f) | |||
| @test_throws OverflowError typemin(Int)//(-1) | |||
There was a problem hiding this comment.
| @test_throws OverflowError typemin(Int)//(-1) | |
| @test_throws OverflowError typemin(Int)//(-1) | |
| @test typemin(Int)//(-2) == abs(typemin(Int)>>1)//1 |
mbauman
changed the title
Co-Authored-By: JeffreySarnoff <JeffreySarnoff@users.noreply.github.com>
base/rational.jl
Outdated
|
|
||
| function Rational{T}(num::Integer, den::Integer) where T<:BitSigned | ||
| num == den == zero(T) && __throw_rational_argerror(T) | ||
| num < 0 && den < 0 && (num === typemin(T) || den === typemin(T)) && __throw_rational_ovferror(num, den) |
There was a problem hiding this comment.
This check may have to move after the divgcd to handle the typemin(Int)//-2 case
There was a problem hiding this comment.
You are right about that. I made the change. I see no distinction between Int < 0 and sign(Int) < 0 in generated code -- is there a reason to prefer one over the other?
There was a problem hiding this comment.
not that I can think of?
There was a problem hiding this comment.
this is an additional gotcha
julia> divgcd(-2,typemin(Int64))
(-1, -4611686018427387904)
julia> Rational{Int}(-2,typemin(Int))
Rational{Int64}(1, -4611686018427387904) # both should be positive
|
The latest revision to rational.jl is passing its tests -- the failures are about other tests being broken. |
|
restart |
|
I just wanted to restart the build -- thinking the stuff that broke the build of Julia has been remedied and, if so this PR likely passes the tests. Usually |
|
perhaps deleting the fork I branched for this work was not the best move ... two files are involved: base/rational.jl and tests/rational.jl please advise |
|
So what's the deal here? You deleted the fork that this branch was from? Fortunately, the patch is still visible so it should be possible to reconstruct the changes from that. |
|
Yes -- the changes are here. |
|
Just make a new PR then, I guess. |
This PR corrects the check for rational overflow when either unary
-orabsis applied to a valueq::Rational{S} where {S::BitSigned} && (numerator(q) == typemin(S) || denominator(q) == typemin(S)).tests have been added