Repository navigation
bpo-29816: Shift operation now has less opportunity to raise OverflowError. - #680
serhiy-storchaka merged 7 commits into
Conversation
Sorry, something went wrong.
…Error. ValueError always is raised rather than OverflowError for negative counts. Shifting zero with non-negative count always returns zero.
|
@serhiy-storchaka, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mdickinson, @tim-one and @brettcannon to be potential reviewers. |
Sorry, something went wrong.
| return NULL; | ||
| if (shiftby >= 0) { | ||
| wordshift = shiftby / PyLong_SHIFT; | ||
| loshift = shiftby - wordshift * PyLong_SHIFT; |
There was a problem hiding this comment.
Why not loshift = shiftby % PyLong_SHIFT;?
Sorry, something went wrong.
There was a problem hiding this comment.
Left shift uses such code. I suppose this is because multiplication and substraction are faster than the "%" operation. If there is a reason to use it for left shift, there is a reason to use it for right shift.
The new code for left and right shifts are almost exact duplicates (there is additional optimization for right shift, but it is not very important and can be omitted). Is it worth to extract it in the helper function?
Sorry, something went wrong.
There was a problem hiding this comment.
I suppose this is because multiplication and substraction are faster than the "%" operation.
This seems unlikely to me. In particular, I'd expect an optimising compiler to be able to recognise and do a good job of optimising the pair shiftby / PyLong_SHIFT and shiftby % PyLong_SHIFT if they appear next to each other in code. Rewriting the % would likely make it harder for the compiler to recognise.
Sorry, something went wrong.
There was a problem hiding this comment.
Is it worth to extract it in the helper function?
That sounds reasonable to me, if you want to do it; if you don't, that also sounds reasonable to me. :-)
Sorry, something went wrong.
mdickinson
left a comment
There was a problem hiding this comment.
Looks okay to me, but if feels a bit convoluted and hard to read / hard to verify. Why not use PyLong arithmetic in the overflow case, for example using divrem1 to extract the number of words / number of bits to shift?
Sorry, something went wrong.
ValueError always is raised rather than OverflowError for negative counts.
Shifting zero with non-negative count always returns zero.