Skip to content

Commit

Permalink
Throw OverflowError on typemin(Int)//(-1) (JuliaLang#51085)
Browse files Browse the repository at this point in the history
Fixes JuliaLang#32443

Currently `typemin(Int)//(-1) == typemin(Int)//(1)`, ignoring an
overflow.

As noted by @JeffreySarnoff in
[JuliaLang#32443](JuliaLang#32443 (comment))
This should throw an error instead of silently overflowing.

To fix this I am using `checked_neg` instead of `-` in the Rational
constructor.

With this PR `(-one(T))//typemin(T)` will now also throw an
`OverflowError` instead of an `ArgumentError`
  • Loading branch information
nhz2 committed Oct 21, 2023
1 parent f71228d commit fb01dd2
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 7 deletions.
6 changes: 2 additions & 4 deletions base/rational.jl
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,10 @@ end
unsafe_rational(num::T, den::T) where {T<:Integer} = unsafe_rational(T, num, den)
unsafe_rational(num::Integer, den::Integer) = unsafe_rational(promote(num, den)...)

@noinline __throw_rational_argerror_typemin(T) = throw(ArgumentError("invalid rational: denominator can't be typemin($T)"))
function checked_den(::Type{T}, num::T, den::T) where T<:Integer
if signbit(den)
den = -den
signbit(den) && __throw_rational_argerror_typemin(typeof(den))
num = -num
den = checked_neg(den)
num = checked_neg(num)
end
return unsafe_rational(T, num, den)
end
Expand Down
11 changes: 8 additions & 3 deletions test/rational.jl
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ using Test
@test (1//typemax(Int)) / (1//typemax(Int)) == 1
@test_throws OverflowError (1//2)^63
@test inv((1+typemin(Int))//typemax(Int)) == -1
@test_throws ArgumentError inv(typemin(Int)//typemax(Int))
@test_throws ArgumentError Rational(0x1, typemin(Int32))
@test_throws OverflowError inv(typemin(Int)//typemax(Int))
@test_throws OverflowError Rational(0x1, typemin(Int32))

@test @inferred(rationalize(Int, 3.0, 0.0)) === 3//1
@test @inferred(rationalize(Int, 3.0, 0)) === 3//1
Expand All @@ -43,10 +43,15 @@ using Test
# issue 26823
@test_throws InexactError rationalize(Int, NaN)
# issue 32569
@test_throws ArgumentError 1 // typemin(Int)
@test_throws OverflowError 1 // typemin(Int)
@test_throws ArgumentError 0 // 0
@test -2 // typemin(Int) == -1 // (typemin(Int) >> 1)
@test 2 // typemin(Int) == 1 // (typemin(Int) >> 1)
# issue 32443
@test Int8(-128)//Int8(1) == -128
@test_throws OverflowError Int8(-128)//Int8(-1)
@test_throws OverflowError Int8(-1)//Int8(-128)
@test Int8(-128)//Int8(-2) == 64

@test_throws InexactError Rational(UInt(1), typemin(Int32))
@test iszero(Rational{Int}(UInt(0), 1))
Expand Down

0 comments on commit fb01dd2

Please sign in to comment.