Repository navigation
MANT: Replace PyObject_Compare with PyObject_RichCompareBool #6265
Description
See #6236 (comment). Since our oldest supported Python version is 2.6, there is no longer a need for separate compilation paths for Python 2.x and 3.x, for Python object comparison, since both provide PyObject_RichCompareBool. It may also be a good idea to get rid of the Pyobject_Cmp function, defined for Python 3.x in npy_3kcompat.h, which was recently discovered to be broken by @pv, see #6229 (comment).
I'll try to put a PR together for this shortly, unless someone else wants the glory for themselves.
Activity
yashmehrotra commentedon Aug 29, 2015
@jaimefrio Hi Jaime, I would like to work on this bug. Can you help me get started ? Apart from removing the
PyObject_Cmp, what all things must be done ?jaimefrio commentedon Aug 29, 2015
I'm having trouble with GitHub finding your user name when trying to do the assignment. But please, consider it assigned and go do it!
yashmehrotra commentedon Aug 29, 2015
@jaimefrio Could you help me a bit with this. I understood that the
PyObject_Cmpmust be removed fromnpy_3kcompat.h. Are there any other changes that I have to look out for ? Also, can you direct me to where I have to make the changes for compilation paths.Sorry for taking your time but frankly saying, this codebase is very big compared to what I used to work with. Thanks.
jaimefrio commentedon Aug 29, 2015
Removing
PyObject_Cmpentirely may require exercising some care, as it may be used by third parties that use NumPy. A quick search of github fornpy_3kcompat.hgives thousands of hits, but only a handful of those seems to also be usingPyObject_Cmp, and those appear to all be clones of the main repository. In any case, sending an e-mail to the list would be in order, outlining the options:But that can be done later in a follow up PR. The more relevant part of this would be to remove all uses of
PyObject_CmpandPyObject_Comparefrom the code base. A quick search shows only a handful of occurrences ofPyObject_Compare, all of which happen inside the#elsepart of an#if defined(NPY_PY3K)block. There may be subtleties, but we basically want to remove the preprocessor instructions and leave the Py3K block only.The one exception is the block holding the single use of
PyObject_Cmpin our code base. That would have to be rewritten in terms ofPyObject_RichCompareBool.Does this make sense?
yashmehrotra commentedon Aug 29, 2015
@jaimefrio Yup. Thanks a lot. Getting started with it right now.
yashmehrotra commentedon Sep 2, 2015
@jaimefrio Any updates on this issue ?
jaimefrio commentedon Sep 2, 2015
Do you follow the mailing list? There was some discussion there on what the right thing to do is, and there is not much of a consensus. Not sure how to go about this...
shoyer commentedon Sep 2, 2015
I agree that raising an exception for now is probably the right way to handle this edge case. It's the least likely to lock us in to any undesired behavior in the future.
seberg commentedon Sep 3, 2015
+1, its true, we may be able to get the 90% version, but if there is no difficulty with avoiding the 10% error margin for the moment, lets just do that :).
yashmehrotra commentedon Sep 3, 2015
@jaimefrio In the discussion from the mailing-list and the discussion here, I guess raising an exception is the way to go, what do you think ?
Also, if you think we should raise an exception, should I go forward with it ?
Merge pull request #6269 from yashmehrotra/depr-pyobject_compare
MAINT: Deprecated PyObject_Compare in favor of PyObject_RichCompareBo…