fix mul_with_overflow for unsigned types larger than 16 bytes#59
Conversation
e55ef6c to
cfe3300
Compare
rfourquet
left a comment
rfourquet
left a comment
There was a problem hiding this comment.
Thanks! LGTM, with a small inline comment to resolve.
Could you please also add a small changelog line to the README, like
### v0.3.7
* fix `mul_with_overflow` for unsigned types larger than 16 bytes ([#59](https://github.com/rfourquet/BitIntegers.jl/pull/59))
And bump the version number. So that i can release the fix after merging.
| for X in XInts | ||
| if sizeof(X) != 3 # bug with [U]Int24, cf. Julia issue #34288 | ||
| for X in (XInts..., Int256, UInt256) | ||
| if VERSION >= v"1.8" || sizeof(X) != 3 # bug with [U]Int24, cf. Julia issue #34288 |
There was a problem hiding this comment.
Actually, this seems to have been fixed earlier, but it doesn't matter because we don't test all the versions.
There was a problem hiding this comment.
The version I put here was based on these two comments: JuliaLang/julia#34288 (comment) and JuliaLang/julia#45460 (comment). I was unable to test with 1.7 locally because Juliaup +1.7 crashes on my machine.
| mul_with_overflow(x::T, y::T) where {T<:XBS} = sizeof(T) >= 16 ? broken_mul_with_overflow(x, y) : checked_smul_int(x, y) | ||
| mul_with_overflow(x::T, y::T) where {T<:XBU} = sizeof(T) >= 16 ? broken_mul_with_overflow(x, y) : checked_umul_int(x, y) | ||
| mul_with_overflow(x::T, y::T) where {T<:XBS} = | ||
| (sizeof(T) > 16 && VERSION < v"1.11-") ? broken_mul_with_overflow(x, y) : checked_smul_int(x, y) |
There was a problem hiding this comment.
Oh, I hadn't noticed you changed sizeof(T) >= 16 to sizeof(T) > 16. Maybe we should be conservative here, and keep >=. In older versions, on some systems, the intrinsic might fail on 128 bit integers?
Also: the code in base/checked.jl suggests that these intrinsics have been prone to bugs, and perhaps some of them still exist on some systems. Let's merge this PR anyway, and wait for possible bug reports to see if more tweaks are needed.
2 participants
Following #48, I believe
mul_with_overflowcan also usechecked_smul_int/checked_umul_int, starting from v1.11.