-
Notifications
You must be signed in to change notification settings - Fork 23
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
Add checked math to FixedDecimals; default to overflow behavior #85
Changes from 9 commits
c81bb83
9da03cd
f19c147
ea46f72
c465e6f
fc1d927
e134ee4
2efb468
7793ffc
30aef4f
07bf40f
137c8b4
e1dd56d
d119dce
41a69fd
3270ad5
4cb2d5d
0cf9092
8eefc80
34a9306
73410da
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -187,13 +187,13 @@ | |
|
||
# these functions are needed to avoid InexactError when converting from the | ||
# integer type | ||
Base.:*(x::Integer, y::FD{T, f}) where {T, f} = reinterpret(FD{T, f}, T(x * y.i)) | ||
Base.:*(x::FD{T, f}, y::Integer) where {T, f} = reinterpret(FD{T, f}, T(x.i * y)) | ||
Base.:*(x::Integer, y::FD{T, f}) where {T, f} = reinterpret(FD{T, f}, *(promote(x, y.i)...)) | ||
Base.:*(x::FD{T, f}, y::Integer) where {T, f} = reinterpret(FD{T, f}, *(promote(x.i, y)...)) | ||
|
||
function Base.:/(x::FD{T, f}, y::FD{T, f}) where {T, f} | ||
powt = coefficient(FD{T, f}) | ||
quotient, remainder = fldmod(widemul(x.i, powt), y.i) | ||
reinterpret(FD{T, f}, T(_round_to_nearest(quotient, remainder, y.i))) | ||
reinterpret(FD{T, f}, _round_to_nearest(quotient, remainder, y.i)) | ||
end | ||
|
||
# These functions allow us to perform division with integers outside of the range of the | ||
|
@@ -202,12 +202,12 @@ | |
powt = coefficient(FD{T, f}) | ||
powtsq = widemul(powt, powt) | ||
quotient, remainder = fldmod(widemul(x, powtsq), y.i) | ||
reinterpret(FD{T, f}, T(_round_to_nearest(quotient, remainder, y.i))) | ||
reinterpret(FD{T, f}, _round_to_nearest(quotient, remainder, y.i)) | ||
end | ||
|
||
function Base.:/(x::FD{T, f}, y::Integer) where {T, f} | ||
quotient, remainder = fldmod(x.i, y) | ||
reinterpret(FD{T, f}, T(_round_to_nearest(quotient, remainder, y))) | ||
reinterpret(FD{T, f}, _round_to_nearest(quotient, remainder, y)) | ||
end | ||
|
||
# integerification | ||
|
@@ -362,14 +362,130 @@ | |
for divfn in [:div, :fld, :fld1, :cld] | ||
# div(x.i, y.i) eliminates the scaling coefficient, so we call the FD constructor. | ||
# We don't need any widening logic, since we won't be multiplying by the coefficient. | ||
@eval Base.$divfn(x::T, y::T) where {T <: FD} = T($divfn(x.i, y.i)) | ||
#@eval Base.$divfn(x::T, y::T) where {T <: FD} = T($divfn(x.i, y.i)) | ||
# @eval Base.$divfn(x::T, y::T) where {T <: FD} = $divfn(promote(x.i, y.i)...) | ||
# TODO(PR): I'm not sure about this one... | ||
# What should it *mean* for `typemax(FD) ÷ FD(0.5)` to overflow? | ||
@eval function Base.$divfn(x::T, y::T) where {T <: FD} | ||
C = coefficient(T) | ||
return reinterpret(T, C * $divfn(promote(x.i, y.i)...)) | ||
end | ||
end | ||
if VERSION >= v"1.4.0-" | ||
# div(x.i, y.i) eliminates the scaling coefficient, so we call the FD constructor. | ||
# We don't need any widening logic, since we won't be multiplying by the coefficient. | ||
Base.div(x::T, y::T, r::RoundingMode) where {T <: FD} = T(div(x.i, y.i, r)) | ||
@eval function Base.div(x::T, y::T, r::RoundingMode) where {T <: FD} | ||
C = coefficient(T) | ||
return reinterpret(T, C * div(x.i, y.i, r)) | ||
end | ||
end | ||
|
||
# --- Checked arithmetic --- | ||
|
||
Base.checked_add(x::FD, y::FD) = Base.checked_add(promote(x, y)...) | ||
Base.checked_sub(x::FD, y::FD) = Base.checked_sub(promote(x, y)...) | ||
Base.checked_mul(x::FD, y::FD) = Base.checked_mul(promote(x, y)...) | ||
Base.checked_div(x::FD, y::FD) = Base.checked_div(promote(x, y)...) | ||
Base.checked_cld(x::FD, y::FD) = Base.checked_cld(promote(x, y)...) | ||
Base.checked_fld(x::FD, y::FD) = Base.checked_fld(promote(x, y)...) | ||
Base.checked_rem(x::FD, y::FD) = Base.checked_rem(promote(x, y)...) | ||
Base.checked_mod(x::FD, y::FD) = Base.checked_mod(promote(x, y)...) | ||
|
||
Base.checked_add(x::FD, y) = Base.checked_add(promote(x, y)...) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also here would be good to audit if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currently, I think that this package just relies on promotion to do arithmetic on BigInts, which I agree is causing unnecessary allocs: julia> @which FD{BigInt,2}(2) + 2
+(x::Number, y::Number)
@ Base promotion.jl:410
julia> @code_typed FD{BigInt,2}(2) + 2
CodeInfo(
1 ─ %1 = invoke Base.GMP.MPZ.set_si(10::Int64)::BigInt
│ %2 = invoke Base.GMP.bigint_pow(%1::BigInt, 2::Int64)::BigInt
│ %3 = invoke Base.GMP.MPZ.mul_si(%2::BigInt, y::Int64)::BigInt
│ %4 = Base.getfield(x, :i)::BigInt
│ %5 = invoke Base.GMP.MPZ.add(%4::BigInt, %3::BigInt)::BigInt
│ %6 = %new(FixedDecimal{BigInt, 2}, %5)::FixedDecimal{BigInt, 2}
└── return %6
) => FixedDecimal{BigInt, 2}
julia> @code_typed optimize=false FD{BigInt,2}(2) + 2
CodeInfo(
1 ─ %1 = Base.:+::Core.Const(+)
│ %2 = Base.promote(x, y)::Tuple{FixedDecimal{BigInt, 2}, FixedDecimal{BigInt, 2}}
│ %3 = Core._apply_iterate(Base.iterate, %1, %2)::FixedDecimal{BigInt, 2}
└── return %3
) => FixedDecimal{BigInt, 2} I'm just going to file this as a future improvement and move on, since I feel bad about how long this PR has lagged for. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Filed: #87. |
||
Base.checked_add(x, y::FD) = Base.checked_add(promote(x, y)...) | ||
Base.checked_sub(x::FD, y) = Base.checked_sub(promote(x, y)...) | ||
Base.checked_sub(x, y::FD) = Base.checked_sub(promote(x, y)...) | ||
Base.checked_mul(x::FD, y) = Base.checked_mul(promote(x, y)...) | ||
Base.checked_mul(x, y::FD) = Base.checked_mul(promote(x, y)...) | ||
Base.checked_div(x::FD, y) = Base.checked_div(promote(x, y)...) | ||
Base.checked_div(x, y::FD) = Base.checked_div(promote(x, y)...) | ||
Base.checked_cld(x::FD, y) = Base.checked_cld(promote(x, y)...) | ||
Base.checked_cld(x, y::FD) = Base.checked_cld(promote(x, y)...) | ||
Base.checked_fld(x::FD, y) = Base.checked_fld(promote(x, y)...) | ||
Base.checked_fld(x, y::FD) = Base.checked_fld(promote(x, y)...) | ||
Base.checked_rem(x::FD, y) = Base.checked_rem(promote(x, y)...) | ||
Base.checked_rem(x, y::FD) = Base.checked_rem(promote(x, y)...) | ||
Base.checked_mod(x::FD, y) = Base.checked_mod(promote(x, y)...) | ||
Base.checked_mod(x, y::FD) = Base.checked_mod(promote(x, y)...) | ||
|
||
function Base.checked_add(x::T, y::T) where {T<:FD} | ||
z, b = Base.add_with_overflow(x.i, y.i) | ||
b && Base.Checked.throw_overflowerr_binaryop(:+, x, y) | ||
return reinterpret(T, z) | ||
end | ||
NHDaly marked this conversation as resolved.
Show resolved
Hide resolved
|
||
function Base.checked_sub(x::T, y::T) where {T<:FD} | ||
z, b = Base.sub_with_overflow(x.i, y.i) | ||
b && Base.Checked.throw_overflowerr_binaryop(:-, x, y) | ||
return reinterpret(T, z) | ||
end | ||
function Base.checked_mul(x::FD{T,f}, y::FD{T,f}) where {T<:Integer,f} | ||
powt = coefficient(FD{T, f}) | ||
quotient, remainder = fldmodinline(widemul(x.i, y.i), powt) | ||
v = _round_to_nearest(quotient, remainder, powt) | ||
typemin(T) <= v <= typemax(T) || Base.Checked.throw_overflowerr_binaryop(:*, x, y) | ||
return reinterpret(FD{T, f}, T(v)) | ||
end | ||
# Checked division functions | ||
for divfn in [:div, :fld, :cld] | ||
@eval function Base.$(Symbol("checked_$divfn"))(x::FD{T,f}, y::FD{T,f}) where {T<:Integer,f} | ||
C = coefficient(FD{T, f}) | ||
# Note: The div() will already throw for divide-by-zero and typemin(T) ÷ -1. | ||
v, b = Base.Checked.mul_with_overflow(C, $divfn(x.i, y.i)) | ||
b && _throw_overflowerr_op($(QuoteNode(divfn)), x, y) | ||
return reinterpret(FD{T, f}, v) | ||
end | ||
end | ||
for remfn in [:rem, :mod] | ||
# rem and mod already check for divide-by-zero and typemin(T) ÷ -1, so nothing to do. | ||
@eval Base.$(Symbol("checked_$remfn"))(x::T, y::T) where {T <: FD} = $remfn(x, y) | ||
end | ||
|
||
_throw_overflowerr_op(op, x::T, y::T) where T = throw(OverflowError("$op($x, $y) overflowed for type $T")) | ||
NHDaly marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
function Base.checked_neg(x::T) where {T<:FD} | ||
r = -x | ||
(x<0) & (r<0) && Base.Checked.throw_overflowerr_negation(x) | ||
return r | ||
end | ||
function Base.checked_abs(x::FD) | ||
r = ifelse(x<0, -x, x) | ||
r<0 || return r | ||
_throw_overflow_abs(x) | ||
end | ||
if VERSION >= v"1.8.0-" | ||
@noinline _throw_overflow_abs(x) = | ||
throw(OverflowError(LazyString("checked arithmetic: cannot compute |x| for x = ", x, "::", typeof(x)))) | ||
else | ||
@noinline _throw_overflow_abs(x) = | ||
throw(OverflowError("checked arithmetic: cannot compute |x| for x = $x")) | ||
end | ||
|
||
# We introduce a new function for this since Base.Checked only supports integers, and ints | ||
# don't have a decimal division operation. | ||
""" | ||
FixedPointDecimals.checked_decimal_division(x::FD, y::FD) -> FD | ||
|
||
Calculates `x / y`, checking for overflow errors where applicable. | ||
|
||
The overflow protection may impose a perceptible performance penalty. | ||
|
||
See also: | ||
- `Base.checked_div` for truncating division. | ||
""" | ||
checked_decimal_division(x::FD, y::FD) = checked_decimal_division(promote(x, y)...) | ||
checked_decimal_division(x, y::FD) = checked_decimal_division(promote(x, y)...) | ||
checked_decimal_division(x::FD, y) = checked_decimal_division(promote(x, y)...) | ||
|
||
function checked_decimal_division(x::FD{T,f}, y::FD{T,f}) where {T<:Integer,f} | ||
powt = coefficient(FD{T, f}) | ||
quotient, remainder = fldmod(widemul(x.i, powt), y.i) | ||
v = _round_to_nearest(quotient, remainder, y.i) | ||
typemin(T) <= v <= typemax(T) || Base.Checked.throw_overflowerr_binaryop(:/, x, y) | ||
return reinterpret(FD{T, f}, v) | ||
end | ||
NHDaly marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
# -------------------------- | ||
|
||
Base.convert(::Type{AbstractFloat}, x::FD) = convert(floattype(typeof(x)), x) | ||
function Base.convert(::Type{TF}, x::FD{T, f}) where {TF <: AbstractFloat, T, f} | ||
convert(TF, x.i / coefficient(FD{T, f}))::TF | ||
|
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.
I think when the
Integer
is aBigInt
, andT
is not, the promote would allocate another bigint which might not be needed because there are usually specialized methods for BigInt x Integer that avoid the allocation.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.
So maybe i should just leave it without the
promote()
and let*
do the promotion internally if needed? I'll try that