Add rdiv_with_overflow and fld_with_overflow #109
Conversation
src/FixedPointDecimals.jl
Outdated
| C = coefficient(FD{T, f}) | ||
| # This case will break the fld call below. This can only happen when f is 0 as y.i | ||
| # cannot be -1 otherwise. | ||
| if T <: Signed && x.i == typemin(T) && y.i == -1 |
There was a problem hiding this comment.
Can you add the f == 0 check here too? I don't know if julia would be able to infer that away?
| if T <: Signed && x.i == typemin(T) && y.i == -1 | |
| if T <: Signed && f == 0 && x.i == typemin(T) && y.i == -1 |
There was a problem hiding this comment.
I don't know what I was thinking but that comment was wrong. I guess I was looking at the integral part of y. y.i can be -1 when you have e.g. FixedDecimal{Int8,2}(-0.01}. Apologies.
There was a problem hiding this comment.
aha, right! 👍
Then we should make sure to test those cases too! Thanks for looking through that!
| @test rdiv_with_overflow(typemin(FD{Int64,0}), FD{Int64,0}(-1)) == (typemin(FD{Int64,0}), true) | ||
|
|
||
| @test fld_with_overflow(FD{Int8,2}(-1), FD{Int8,2}(0.9)) == (FD{Int8,2}(0.56), true) | ||
| @test fld_with_overflow(typemin(FD{Int64,0}), FD{Int64,0}(-1)) == (typemin(FD{Int64,0}), true) |
There was a problem hiding this comment.
per above, please add a case of non-0 f:
@test fld_with_overflow(typemin(FD{Int64,2}), FD{Int64,2}(-1)) == (typemin(FD{Int64,2}), true)
or somethign like that
There was a problem hiding this comment.
Added this for rdiv_with_overflow an equivalent for fld (it doesn't overflow).
There was a problem hiding this comment.
oops! Right, i meant FD{Int64,2}(-0.01) -- that is, i want to exercise the overflow case for a non-0 f.
I think it should be this:
julia> FixedPointDecimals.fld_with_overflow(typemin(FD{Int8,2}), FD{Int8,2}(-0.01))
(FixedDecimal{Int8,2}(-1.28), true)There was a problem hiding this comment.
I've added the two test cases in PR review comment suggestions :)
No description provided.