avoids integer overflow on left shift#106
Conversation
|
whoops, accidentally built this on an old master branch. ignore until I can check out the updates and rebase... |
in several places the conversion machinery uses `1<<f`, which is a
problem for `Fixed{Int64, 63}`, and `Fixed{Int32, 31}` on 32-bit machines.
This changes those to `one(widen1(T))<<f`.
Fixes JuliaMath#104.
|
ok, rebased on latest master |
Codecov Report
@@ Coverage Diff @@
## master #106 +/- ##
==========================================
- Coverage 91.7% 89.52% -2.19%
==========================================
Files 4 4
Lines 193 191 -2
==========================================
- Hits 177 171 -6
- Misses 16 20 +4
Continue to review full report at Codecov.
|
|
It looks like the test coverage reduction has something to do with whether the tests get run in 0.6 or 0.7, not the changes in this PR. |
|
The code seems fine, but those tests you added pass on current master, which means they're not actually testing this fix. These days I try to remember to write the tests first, and then fix the code, just so I can be sure I'm testing what I think I am. |
|
The tests fail in master on 32-bit. You can test with the 32-bit Linux builds. |
|
Though it does seem a little strange that the Fixed{Int64, 63} tests pass on 64-bit master now that I think about it. I would expect those to overflow as well... |
|
Ah, of course, thanks for reminding me. On master I got this: julia> typemax(Fixed{Int64, 63})
-1.0Q0f63and with this PR julia> typemax(Fixed{Int64, 63})
1.0Q0f63 Perhaps add that to the test suite? |
|
OK, fixed up the tests so they explicitly test for The new tests now fail on 64-bit |
|
Thanks so much! |
|
no prob! FixedPointNumbers is one of my favorite Julia packages, glad I could help out. I'm hoping this can get tagged relatively soon because it's blocking me merging a PR on one of my packages whose tests were failing on 32-bit. :) |
|
0.4.6 has been submitted, thanks for the reminder. |
3 participants
in several places the conversion machinery uses
1<<f, which is aproblem for
Fixed{Int64, 63}, andFixed{Int32, 31}on 32-bit machines.This changes those to
one(widen1(T))<<f.Fixes #104.