-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reorganize type hierarchy such that SafeSigned <: Signed, SafeUnsigned <:Unsigned #20
Reorganize type hierarchy such that SafeSigned <: Signed, SafeUnsigned <:Unsigned #20
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in promote.jl, please amend the revision to be
promote_rule(::Type{S1}, ::Type{S2}) where {S1<:SafeSigned, S2<:SafeSigned} =
ifelse(sizeof(S1) < sizeof(S2), S2, S1)
promote_rule(::Type{U1}, ::Type{U2}) where {U1<:SafeUnsigned, U2<:SafeUnsigned} =
ifelse(sizeof(U1) < sizeof(U2), U2, U1)
promote_rule(::Type{S}, ::Type{U}) where {S<:SafeSigned, U<:SafeUnsigned} =
ifelse(sizeof(U) < sizeof(S), S, U)
promote_rule(::Type{S}, ::Type{T}) where {S<:SafeSigned, T<:Signed} = S
promote_rule(::Type{S}, ::Type{T}) where {S<:SafeSigned, T<:Unsigned} = S
promote_rule(::Type{S}, ::Type{T}) where {S<:SafeUnsigned, T<:Signed} = S
promote_rule(::Type{S}, ::Type{T}) where {S<:SafeUnsigned, T<:Unsigned} = S
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in binary_ops.jl
it looks like in addition to
@inline function $OP(x::T1, y::T2) where {T1<:SafeSigned, T2<:Signed} ...
we need
@inline function $OP(x::T1, y::T2) where {T1<:SafeUnsigned T2<:Unsigned} ...
and
@inline function $OP(x::T1, y::T2) where {T1<:SafeInteger, T2<:Integer} ...
(I do not want to use the last one in place of the foregoing two,
because keeping the cases separate makes reasoning about it follow
the same logic as used elsewhere -- maybe changing everything makes sense,
it would take a bunch of benchmarking and MethodInstances tracking
to be sure of that .. so for a later rev perhaps)
if so, please make that change
I think we already have that SaferIntegers.jl/src/binary_ops.jl Line 72 in 38dd988
|
I'm a bit confused on the changes you requested for promote_rules: If I replace promote_rule(::Type{S}, ::Type{T}) where S<:SafeSigned where T<:SafeSigned =
safeint(promote_type(baseint(S), baseint(T)))
promote_rule(::Type{S}, ::Type{T}) where S<:SafeSigned where T<:SafeUnsigned =
safeint(promote_type(baseint(S), baseint(T)))
promote_rule(::Type{S}, ::Type{T}) where S<:SafeUnsigned where T<:SafeUnsigned =
safeint(promote_type(baseint(S), baseint(T)))
promote_rule(::Type{S}, ::Type{T}) where S<:SafeSigned where T<:Signed =
T <: SafeInteger ? Base.Bottom : S
promote_rule(::Type{S}, ::Type{T}) where S<:SafeSigned where T<:Unsigned =
T <: SafeInteger ? Base.Bottom : S
promote_rule(::Type{S}, ::Type{T}) where S<:SafeUnsigned where T<:Signed =
T <: SafeInteger ? Base.Bottom : S
promote_rule(::Type{S}, ::Type{T}) where S<:SafeUnsigned where T<:Unsigned =
T <: SafeInteger ? Base.Bottom : S with promote_rule(::Type{S1}, ::Type{S2}) where {S1<:SafeSigned, S2<:SafeSigned} =
ifelse(sizeof(S1) < sizeof(S2), S2, S1)
promote_rule(::Type{U1}, ::Type{U2}) where {U1<:SafeUnsigned, U2<:SafeUnsigned} =
ifelse(sizeof(U1) < sizeof(U2), U2, U1)
promote_rule(::Type{S}, ::Type{U}) where {S<:SafeSigned, U<:SafeUnsigned} =
ifelse(sizeof(U) < sizeof(S), S, U)
promote_rule(::Type{S}, ::Type{T}) where {S<:SafeSigned, T<:Signed} = S
promote_rule(::Type{S}, ::Type{T}) where {S<:SafeSigned, T<:Unsigned} = S
promote_rule(::Type{S}, ::Type{T}) where {S<:SafeUnsigned, T<:Signed} = S
promote_rule(::Type{S}, ::Type{T}) where {S<:SafeUnsigned, T<:Unsigned} = S Then I get the following checked mixed arithmetic: Error During Test at C:\Users\kittisopikulm\.julia\dev\SaferIntegers\test\runtests.jl:385
Test threw exception
Expression: SaferIntegers.checked_mul(SafeInt32(7), SafeUInt16(2)) === SafeInt32(7 * 2)
StackOverflowError:
Stacktrace:
[1] promote_type
@ .\promotion.jl:233 [inlined]
[2] promote_result(#unused#::Type, #unused#::Type, #unused#::Type{SafeInt32}, #unused#::Type{SafeUInt16})
@ Base .\promotion.jl:247
--- the last 2 lines are repeated 79982 more times ---
[159967] promote_type
@ .\promotion.jl:233 [inlined]
checked mixed arithmetic: Error During Test at C:\Users\kittisopikulm\.julia\dev\SaferIntegers\test\runtests.jl:386
Test threw exception
Expression: SaferIntegers.checked_div(SafeInt32(7), SafeUInt16(2)) === SafeInt32(div(7, 2))
StackOverflowError:
Stacktrace:
[1] promote_type
@ .\promotion.jl:233 [inlined]
[2] promote_result(#unused#::Type, #unused#::Type, #unused#::Type{SafeInt32}, #unused#::Type{SafeUInt16})
@ Base .\promotion.jl:247
--- the last 2 lines are repeated 79982 more times ---
[159967] promote_type
@ .\promotion.jl:233 [inlined]
checked mixed arithmetic: Error During Test at C:\Users\kittisopikulm\.julia\dev\SaferIntegers\test\runtests.jl:387
Test threw exception
Expression: SaferIntegers.checked_rem(SafeInt32(7), SafeUInt16(2)) === SafeInt32(rem(7, 2))
StackOverflowError:
Stacktrace:
[1] promote_type
@ .\promotion.jl:233 [inlined]
[2] promote_result(#unused#::Type, #unused#::Type, #unused#::Type{SafeInt32}, #unused#::Type{SafeUInt16})
@ Base .\promotion.jl:247
--- the last 2 lines are repeated 79982 more times ---
[159967] promote_type
@ .\promotion.jl:233 [inlined]
checked mixed arithmetic: Error During Test at C:\Users\kittisopikulm\.julia\dev\SaferIntegers\test\runtests.jl:388
Test threw exception
Expression: SaferIntegers.checked_mod(SafeInt32(7), SafeUInt16(2)) === SafeInt32(mod(7, 2))
StackOverflowError:
Stacktrace:
[1] promote_type
@ .\promotion.jl:233 [inlined]
[2] promote_result(#unused#::Type, #unused#::Type, #unused#::Type{SafeInt32}, #unused#::Type{SafeUInt16})
@ Base .\promotion.jl:247
--- the last 2 lines are repeated 79982 more times ---
[159967] promote_type
@ .\promotion.jl:233 [inlined]
checked mixed arithmetic: Error During Test at C:\Users\kittisopikulm\.julia\dev\SaferIntegers\test\runtests.jl:389
Test threw exception
Expression: SaferIntegers.checked_fld(SafeInt32(7), SafeUInt16(2)) === SafeInt32(fld(7, 2))
StackOverflowError:
Stacktrace:
[1] promote_type
@ .\promotion.jl:233 [inlined]
[2] promote_result(#unused#::Type, #unused#::Type, #unused#::Type{SafeInt32}, #unused#::Type{SafeUInt16})
@ Base .\promotion.jl:247
--- the last 2 lines are repeated 79982 more times ---
[159967] promote_type
@ .\promotion.jl:233 [inlined]
checked mixed arithmetic: Error During Test at C:\Users\kittisopikulm\.julia\dev\SaferIntegers\test\runtests.jl:390
Test threw exception
Expression: SaferIntegers.checked_cld(SafeInt32(7), SafeUInt16(2)) === SafeInt32(cld(7, 2))
StackOverflowError:
Stacktrace:
[1] promote_type
@ .\promotion.jl:233 [inlined]
[2] promote_result(#unused#::Type, #unused#::Type, #unused#::Type{SafeInt32}, #unused#::Type{SafeUInt16})
@ Base .\promotion.jl:247
--- the last 2 lines are repeated 79982 more times ---
[159967] promote_type
@ .\promotion.jl:233 [inlined]
Test Summary: | Pass Error Total
checked mixed arithmetic | 10 6 16
ERROR: LoadError: Some tests did not pass: 10 passed, 0 failed, 6 errored, 0 broken.
in expression starting at C:\Users\kittisopikulm\.julia\dev\SaferIntegers\test\runtests.jl:373
ERROR: Package SaferIntegers errored during testing |
I added this in 1080c72 |
I am pleased to know SaferIntegers is in your safe purview. I am merging. |
This pull request is my attempt to implement the following:
The unmodified tests currently pass with this and the associated changes.