This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

Author Oren Milman
Recipients Oren Milman
Date 2016-06-04.19:54:26
SpamBayes Score -1.0
Marked as misclassified Yes
Message-id <1465070067.21.0.305651029523.issue27222@psf.upfronthosting.co.za>
In-reply-to
Content
------------ current state ------------
1. long_rshift first checks whether a is a negative int. If it is, it does (edited for brevity) 'z = long_invert(long_rshift(long_invert(a), b));'.
Otherwise, it calculates the result of the shift and stores it in z. In this block, there is a check whether a is a negative int.

The second check was always there - since revision 443, in which long_rshift was first added. However, in revision 590, the first aforementioned check (whether a is a negative int), along with a (edited for brevity) 'return long_invert(long_rshift(long_invert(a), b));' were added, but the second check whether a is a negative int wasn't removed, and remained there to this day.

2. Ultimately, long_rshift does 'return (PyObject *) maybe_small_long(z);' for both cases (whether a is a negative int or not). 
Calling maybe_small_long in case a is a negative int is redundant, as long_invert returns (in case it doesn't fail) by either doing 'return PyLong_FromLong(-(MEDIUM_VALUE(v)+1));' or 'return (PyObject *)maybe_small_long(x);'. In both cases, long_invert would return a reference to an element of small_ints if it should.

Calls to maybe_small_long were added both to long_rshift and long_invert in revision 48567, as part of an effort to wipe out different places in the code where small_ints could be used (to save up memory), but was not. I am not sure why maybe_small_long was added to long_rshift where it would be redundant in case a is a negative int.

3. In different cases of failure, long_rshift does 'goto rshift_error;'. 
The destination of these goto statements is actually a return statement that would also be reached in almost any case of success (except for a certain case in which the result of the shift is obviously zero).

That goto was added in revision 15725. Back then, CONVERT_BINOP was added, and calling it in long_rshift required calling Py_DECREF for a and b before returning.
Later on, in revision 43313, CONVERT_BINOP was removed, along with the calls to Py_DECREF it required, but the goto was left untouched, and remained there to this day.


------------ proposed changes ------------
All of the proposed changes are in Objects/longobject.c in long_rshift:
    1. Remove the check whether a is a negative int in the block that gets executed in case a is not a negative int.
    
    2. Move the call to maybe_small_long inside the block that runs in case a is not a negative int.

    3. Replace every 'goto rshift_error;' with a 'return NULL;', and remove the rshift_error label. 

    I could have kept the goto statements, with 'return (PyObject *)z;' as their destination, but I believe it would have been less clear. Also, there are many similar places in longobject.c where 'return NULL;' is done on failure.
    

------------ diff ------------
The proposed patches diff file is attached.


------------ tests ------------
I built the patched CPython for x86, and played with it a little. Everything seemed to work as usual. 
Specifically, I did:
    for i in range(10000):
        if not all(smallInt is ((smallInt << i) >> i) for (
                smallInt) in range(-5, 257)):
            break
    print(i)
And indeed 9999 was printed.

In addition, I ran 'python_d.exe -m test -j3' (on my 64-bit Windows 10) with and without the patches, and got quite the same output.
the outputs of both runs are attached.
History
Date User Action Args
2016-06-04 19:54:27Oren Milmansetrecipients: + Oren Milman
2016-06-04 19:54:27Oren Milmansetmessageid: <1465070067.21.0.305651029523.issue27222@psf.upfronthosting.co.za>
2016-06-04 19:54:27Oren Milmanlinkissue27222 messages
2016-06-04 19:54:26Oren Milmancreate