thouis / numpy-trac-migration

numpy Trac to github issues migration
2 stars 3 forks source link

typedef npy_intp to Py_ssize_t (cf. PEP353) (migrated from Trac #1056) #2609

Closed thouis closed 11 years ago

thouis commented 11 years ago

Original ticket http://projects.scipy.org/numpy/ticket/1056 Reported 2009-03-15 by atmention:sturlamolden, assigned to unknown.

arrayobject.h contains these typedefs:

typedef Py_intptr_t npy_intp; typedef Py_uintptr_t npy_uintp;

From PEP 353: "Conceptually, Py_intptr_t and Py_ssize_t are different things: Py_intptr_t needs to be the same size as void*, and Py_ssize_t the same size as size_t. These could differ, e.g. on machines where pointers have segment and offset. On current flat-address space machines, there is no difference, so for all practical purposes, Py_intptr_t would have worked as well."

This would thus be preferred, although the difference will not be noticeable on most platforms:

typedef Py_ssize_t npy_intp; typedef size_t npy_uintp;

thouis commented 11 years ago

Comment in Trac by atmention:charris, 2009-03-15

Py_ssize_t was defined in python 2.5. The compatable backport to python version 2.4 is is to define it to int, which is not compatible with npy_intp. So as long as we support python 2.4 defining npy_intp to Py_ssize_t is not useful. I suppose the definition could check for the python version.

thouis commented 11 years ago

Comment in Trac by atmention:sturlamolden, 2009-03-15

Replying to [comment:1 charris]:

Py_ssize_t was defined in python 2.5. The compatable backport to python version 2.4 is is to define it to int, which is not compatible with npy_intp. So as long as we support python 2.4 defining npy_intp to Py_ssize_t is not useful. I suppose the definition could check for the python version.

ndarrayobject.h defines Py_ssize_t to int for Python 2.4. But this define comes after the npy_intp typedef, so their order must be reversed. Something like this:

ifdef constchar

undef constchar

endif

if (PY_VERSION_HEX < 0x02050000)

ifndef PY_SSIZE_T_MIN

typedef int Py_ssize_t;
#define PY_SSIZE_T_MAX INT_MAX
#define PY_SSIZE_T_MIN INT_MIN

endif

define NPY_SSIZE_T_PYFMT "i"

undef PyIndex_Check

define constchar const char

define PyIndex_Check(op) 0

else

define NPY_SSIZE_T_PYFMT "n"

define constchar char

endif

typedef Py_ssize_t npy_intp; typedef size_t npy_uintp; /*

define NPY_SIZEOF_INTP NPY_SIZEOF_PY_INTPTR_T

define NPY_SIZEOF_UINTP NPY_SIZEOF_PY_INTPTR_T

*/

define NPY_SIZEOF_INTP sizeof(size_t)

define NPY_SIZEOF_UINTP sizeof(size_t)

thouis commented 11 years ago

Comment in Trac by atmention:sturlamolden, 2009-03-15

/* This is to typedef npy_intp to the appropriate pointer size for this
 * platform.  Py_intptr_t, Py_uintptr_t are defined in pyport.h. */

#ifdef constchar
#undef constchar
#endif

#if (PY_VERSION_HEX < 0x02050000)
  #ifndef PY_SSIZE_T_MIN
    typedef int Py_ssize_t;
    #define PY_SSIZE_T_MAX INT_MAX
    #define PY_SSIZE_T_MIN INT_MIN
  #endif
#define NPY_SSIZE_T_PYFMT "i"
#undef PyIndex_Check
#define constchar const char
#define PyIndex_Check(op) 0
#else
#define NPY_SSIZE_T_PYFMT "n"
#define constchar char
#endif

typedef Py_ssize_t npy_intp;
typedef size_t npy_uintp;
#define NPY_SIZEOF_INTP sizeof(size_t)
#define NPY_SIZEOF_UINTP sizeof(size_t)
thouis commented 11 years ago

Comment in Trac by atmention:cournape, 2009-03-16

May I ask what is the problem ? I see answers, but I don't understand the question :)

thouis commented 11 years ago

Comment in Trac by atmention:sturlamolden, 2009-03-16

The problem is: sizeof(npy_intp) should be sizeof(size_t), not sizeof(void*).

sizeof(size_t) and sizeof(void*) are equivalent only on systems with flat address space.

This is why Python uses Py_ssize_t instead of Py_intptr_t for indexing sequences.

thouis commented 11 years ago

Comment in Trac by atmention:cournape, 2009-03-19

sizeof(npy_intp) should certainly be sizeof(void*), since npy_intp is by definition a signed integer which is big enough to hold a pointer.

The problem is that we use npy_intp for indexing, not the definition of npy_intp. Since we can't yet rely on Py_ssize_t, the only solution I can see would be to get our own npy_ssize_t (which would have the same size as ssize_t).

thouis commented 11 years ago

Comment in Trac by atmention:cournape, 2009-03-19

thouis commented 11 years ago

Comment in Trac by atmention:charris, 2009-03-19

I think we setting out to solve a problem that doesn't yet exist. In the paleocyber age there were computers that couldn't use the registers to address sufficient memory, so they used segment registers and such. IIRC, the segment registers had to be loaded separately from the "pointers". But there aren't many of those machines in use today outside of museums. I say if we want to make this change, wait a few years, require python > 2.4, and define npy_intp to Py_ssize_t for backward compatibility. In any case, npy_intp has always been intended as an indexing integer, so just change the documentation of to say so. How we get the appropriate size doesn't really matter. We could define a npy_ssize_t, but I don't think we need to, just start using Py_ssize_t when the time is right.

thouis commented 11 years ago

Comment in Trac by atmention:cournape, 2009-03-20

When I say npy_intp_t is by definition an int big enough to get a void*, this should actually refer to intp_t. Having a npy_ssize_t should follow if only for consistency, no ? I mean, what's the point of having npy_intp_t, etc... ? I always found using npy_intp_t for indexing confusing myself.

thouis commented 11 years ago

Comment in Trac by atmention:charris, 2009-03-20

What's in a name? The name was poorly chosen, no doubt reflecting python itself, but that doesn't change its usage. And changing it would break a lot of other peoples' code. I say leave it alone and add an alias for numpy if it is truly unbearable.

thouis commented 11 years ago

Comment in Trac by atmention:cournape, 2009-03-20

I guess we are misunderstanding each other - I am arguing against changing npy_intp :) We should use Py_ssize_t for indexing, not npy_intp. And that's how it is done internally in numpy currently, AFAICS, although not consistently (grep tells me there are many more Py_ssize_t variables than npy_intp in numpy/core/src).

As you said, we can't easily change npy_intp to Py_ssize_t arguments in current public API as it would break almost any extension out there. But if we were to change things in an incompatible manner, we should change things such as not to use npy_intp for indexing instead of making a broken definition of npy_intp.

thouis commented 11 years ago

Comment in Trac by atmention:cournape, 2009-03-20

I think Travis, Robert or someone else familiar with the buffer interface and what it would mean for numpy should make this decision. In py3k, it looks like Py_ssize_t is used for the members which are common with the current code in ndarrayobject.h:

http://docs.python.org/3.0/c-api/buffer.html?highlight=buffer#Py_buffer

But I really don't know anything about buffer API.

thouis commented 11 years ago

Comment in Trac by atmention:charris, 2009-03-20

Grep for intp instead, Py_ssize_t is pretty much restricted to python calls. I count 689 lines containing intp in numpy/core/src. Note

include/numpy/noprefix.h:#define intp npy_intp

Chuck

thouis commented 11 years ago

Comment in Trac by atmention:cournape, 2009-03-20

Hm, right. Well, it kind of makes the point of inconsistency stronger :)

thouis commented 11 years ago

Comment in Trac by atmention:charris, 2009-03-20

Yes, it is a blemish, the poor symbol suffers from reverse Hungarian notation. But as tolerant, cosmopolitan sorts we can overlook this cosmetic defect and see the true meaning that lies beneath.

Just sayin'. It would be nice if we had consistent symbols everywhere but I don't think it is a pressing concern and we will have to keep the old symbols around for backward compatibility anyway.

thouis commented 11 years ago

Comment in Trac by atmention:mwiebe, 2011-03-24

I agree with the comments that something saying "intp" in its name should be the size of void*. If we want to follow Py_ssize_t, that should be an explicit decision changing to use a different typedef everywhere.