BUG: Check for errors when PyInt_AsLong is called in np.random #8884
Conversation
| @@ -16,7 +16,7 @@ cdef extern from "Python.h": | |||
|
|
|||
| # Float API | |||
| double PyFloat_AsDouble(object ob) except? -1.0 | |||
| long PyInt_AsLong(object ob) | |||
| long PyInt_AsLong(object ob) except? -1 | |||
There was a problem hiding this comment.
PyInt_AsLong may have thrown an exception if it returns -1
https://docs.python.org/2/c-api/int.html#c.PyInt_AsLong
numpy/random/tests/test_random.py
Outdated
|
|
||
| class ThrowingInteger(np.ndarray): | ||
| def __int__(self): | ||
| raise TypeError |
There was a problem hiding this comment.
Thanks for doing this, just a minor nitpick...
Someone seeing this test and the one you wrote for PyFloat_AsDouble may think there's something fundamentally different between both cases since the exceptions are different. So it would probably be good to make both more equal by having both raise the same error.
I also think it would be good to group them both in a test_scalar_exception_propagation method, rather than have them grouped with the random method used for testing, but this may be a matter of personal taste.
After numpy#8883 was merged it was noticed that the same problem was occuring with calls to PyInt_AsLong. Namely that PyErr_Occoured wasn't being checked if it returned -1 indicating an exception could have been thrown. This PR adds those checks as well as a regression test.
simongibbons
force-pushed
the
add-python-cython-error-checking
branch
from
f1875cc
to
30ab8fc
Compare
|
@jaimefrio I've updated the tests taking your comments onboard. Note that the change in exception was intentional as there was some checks that all of the integers passed into I agree the tests should be combined though and have done. |
|
Looks much better to me now. Thanks, in it goes! |
After #8883 was merged it was noticed that the same problem was
occuring with calls to PyInt_AsLong. Namely that PyErr_Occoured
wasn't being checked if it returned -1 indicating an exception
could have been thrown.
This PR adds those checks as well as a regression test.