bpo-29816: Shift operation now has less opportunity to raise OverflowError.

[serhiy-storchaka]

ValueError always is raised rather than OverflowError for negative counts.
Shifting zero with non-negative count always returns zero.

[mention-bot]

@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.

[mdickinson]

Why not loshift = shiftby % PyLong_SHIFT;?

[serhiy-storchaka]

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?

[mdickinson]

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.

[mdickinson]

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. :-)

[mdickinson]

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?