BUG: don't pollute public namespace with customized PyIndex_Check on Py2.4 #307

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants
@pv
Member

pv commented Jun 11, 2012

Move compatibility #define for PyIndex_Check on Python 2.4 to a private compat header

The problem is that including the public Numpy header "arrayobject.h" defines a compatibility wrapper for "PyIndex_Check" on Python 2.4. Moreover, this compatibility wrapper functions incorrectly... This #define probably should not be in the public namespace, as it already causes problems with Cython on Py2.4 (which we hit into in Scipy release):
http://permalink.gmane.org/gmane.comp.python.cython.devel/13983

The second commit moves all non Py3-stuff out of npy_3compat.h to another, more private compat header, for stylistic reasons (mainly because #defining PyIndex_Check to 0 rather than something like PyNumber_Check is possibly not the correct solution, but I didn't want to go changing it inside Numpy for this).

A question with this PR is --- is there 3rd party software out there that relies on Numpy headers defining the otherwise missing PyIndex_Check being defined, and do we care. I'd say we don't care, this seems like a corner case, and the current behavior in Numpy is wrong.

pv added some commits Jun 11, 2012

BUG: core: don't pollute public namespace with PyIndex_Check on Pytho…
…n 2.4

Public numpy headers shouldn't define a replacement PyIndex_Check.  This
causes unexpected behavior in Python2.4 in 3rd party code.
STY: core: move non-Py3 specific stuff out from npy_3kcompat.h to pri…
…vate npy_pycompat.h

npy_3kcompat.h is semi-private, so this can be done.
@charris

This comment has been minimized.

Show comment
Hide comment
@charris

charris Jun 11, 2012

Member

Looks good to me. Should we also move python version dependent things like the DEPRECATE macro?

Member

charris commented Jun 11, 2012

Looks good to me. Should we also move python version dependent things like the DEPRECATE macro?

@pv

This comment has been minimized.

Show comment
Hide comment
@pv

pv Jun 11, 2012

Member

If they're not part of the public API, why not. I guess DEPRECATE is not, at least Scipy doesn't use it.

Member

pv commented Jun 11, 2012

If they're not part of the public API, why not. I guess DEPRECATE is not, at least Scipy doesn't use it.

@teoliphant

This comment has been minimized.

Show comment
Hide comment
@teoliphant

teoliphant Jun 11, 2012

Member

It might be time to deprecate Python 2.4 support as well. The thinking behind the PyIndex_Check being set to 0 is that before 2.4 there is no Index attribute and so no object can be an "index" object. This is the correct behavior. It is Cython that is doing the wrong thing here.

In practice, it depends on why people are using PyIndex_Check in the code. An index is anything that can be interpreted as an integer. A number is quite a bit more general than that. Code might be trying to access the tp_index slot in the number structure after doing a PyIndex_Check. You have to return 0 if there is no such slot (like in Python 2.4).

Cython code should not be using PyIndex_Check as it if were the same thing as PyNumber_Check. That is a mistake. But, I'm not going to try and fix that problem, as they must not be accessing the tp_index slot directly.

It's fine to make our version private, though --- especially because it's just a Python 2.4 thing.

Member

teoliphant commented Jun 11, 2012

It might be time to deprecate Python 2.4 support as well. The thinking behind the PyIndex_Check being set to 0 is that before 2.4 there is no Index attribute and so no object can be an "index" object. This is the correct behavior. It is Cython that is doing the wrong thing here.

In practice, it depends on why people are using PyIndex_Check in the code. An index is anything that can be interpreted as an integer. A number is quite a bit more general than that. Code might be trying to access the tp_index slot in the number structure after doing a PyIndex_Check. You have to return 0 if there is no such slot (like in Python 2.4).

Cython code should not be using PyIndex_Check as it if were the same thing as PyNumber_Check. That is a mistake. But, I'm not going to try and fix that problem, as they must not be accessing the tp_index slot directly.

It's fine to make our version private, though --- especially because it's just a Python 2.4 thing.

@pv

This comment has been minimized.

Show comment
Hide comment
@pv

pv Jun 12, 2012

Member

As far as I see, Cython is using PyIndex_Check only to check if object can be used for indexing, and in that case functional backward compatibility is easiest to achieve via PyNumber_Check && !PyFloat_Check && !PyComplex_Check. Trying to access the tp_index slot on Python2.4 causes a compile error, I think.

EDIT: uhh, or maybe the "correct" compatibility mode should be PyInt_Check on Py2.4. I'm not sure what Cython should do here...

Member

pv commented Jun 12, 2012

As far as I see, Cython is using PyIndex_Check only to check if object can be used for indexing, and in that case functional backward compatibility is easiest to achieve via PyNumber_Check && !PyFloat_Check && !PyComplex_Check. Trying to access the tp_index slot on Python2.4 causes a compile error, I think.

EDIT: uhh, or maybe the "correct" compatibility mode should be PyInt_Check on Py2.4. I'm not sure what Cython should do here...

+ * Accessing items of ob_base
+ */
+
+#if (PY_VERSION_HEX < 0x02060000)

This comment has been minimized.

@charris

charris Jul 13, 2012

Member

Aren't these needed for Scipy?

@charris

charris Jul 13, 2012

Member

Aren't these needed for Scipy?

This comment has been minimized.

@pv

pv Jul 13, 2012

Member

None of them is used in Scipy --- apart from Cython/Swig generated files which have their own versions.

@pv

pv Jul 13, 2012

Member

None of them is used in Scipy --- apart from Cython/Swig generated files which have their own versions.

@charris

This comment has been minimized.

Show comment
Hide comment
@charris

charris Jul 13, 2012

Member

OK. The new stuff in #354 should probably go into the new file.

Member

charris commented Jul 13, 2012

OK. The new stuff in #354 should probably go into the new file.

@njsmith

This comment has been minimized.

Show comment
Hide comment
@njsmith

njsmith Jul 13, 2012

Member

Github says this has a merge conflict, but other than that, it sounds ready to go?

Member

njsmith commented Jul 13, 2012

Github says this has a merge conflict, but other than that, it sounds ready to go?

@charris

This comment has been minimized.

Show comment
Hide comment
@charris

charris Jul 13, 2012

Member

A rebase should fix the conflict, then I think we can put it in.

Member

charris commented Jul 13, 2012

A rebase should fix the conflict, then I think we can put it in.

@charris

This comment has been minimized.

Show comment
Hide comment
@charris

charris Jul 13, 2012

Member

Let's do 354 first, then fix this one to move the new functions into npy_compat. Sound good?

Member

charris commented Jul 13, 2012

Let's do 354 first, then fix this one to move the new functions into npy_compat. Sound good?

@charris

This comment has been minimized.

Show comment
Hide comment
@charris

charris Jul 13, 2012

Member

OK, I merged 354, so this needs a fixup.

Member

charris commented Jul 13, 2012

OK, I merged 354, so this needs a fixup.

@charris

This comment has been minimized.

Show comment
Hide comment
@charris

charris Jul 13, 2012

Member

Fixed up in PR 355.

Member

charris commented Jul 13, 2012

Fixed up in PR 355.

@charris

This comment has been minimized.

Show comment
Hide comment
@charris

charris Jul 14, 2012

Member

Rebased version committed.

Member

charris commented Jul 14, 2012

Rebased version committed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment