Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API Make PyArray_DATA return void* #2780

Merged
merged 1 commit into from Dec 4, 2012

Conversation

@luispedro
Copy link
Contributor

commented Dec 3, 2012

PyArray_DATA is documented as returning void_. Changing it to return
char_ breaks code such as (in C++)::

float* my_floats = static_cast<float*>(PyArray_DATA(my_array));

PyArray_BYTES returns char*, but is otherwise the same function.

This was previously discussed here: abf0489
as it caused a compilation error in mahotas:
luispedro/mahotas#26

My main argument is that http://docs.scipy.org/doc/numpy/reference/c-api.array.html clearly states that PyArray_DATA returns void* and unless there is a strong reason against changing the public API, it should not change. In this case, there is even a perfect alternative for those who wish to have the char*: PyArray_BYTES.

API Make PyArray_DATA return void*
PyArray_DATA is documented as returning void*. Changing it to return
char* breaks code such as (in C++)::

    float* my_floats = static_cast<float*>(PyArray_DATA(my_array));

PyArray_BYTES returns char*, but is otherwise the same function.
@njsmith

This comment has been minimized.

Copy link
Member

commented Dec 3, 2012

IIUC this was introduced by gh-2709, which was to fix a bug encountered in Theano. But perhaps the PyArray_DATA part isn't an issue for them, only PyArray_BYTES. @nouiz, thoughts?

@luispedro

This comment has been minimized.

Copy link
Contributor Author

commented Dec 3, 2012

This makes PyArray_DATA be what it always was and what it always was documented to be.

@charris

This comment has been minimized.

Copy link
Member

commented Dec 3, 2012

I believe the theano folks used PyArray_BYTES, I was responsible for the PyArray_DATA change. Numpy 1.6 didn't have the cast in PyArray_BYTES, it was new in 1.7. @nouiz, can you confirm that you only need to use PyArray_BYTES?

If we make this change, then the corresponding function should probably return void* also. I'm not a big fan of void* but we should probably keep backwards compatibility here. We still need a function to do the equivalent of std::vector swap, but that isn't pressing.

@luispedro

This comment has been minimized.

Copy link
Contributor Author

commented Dec 3, 2012

This patch changes both the macro and the function (and also adds PyArray_BYTES as a separate function returning char*).

@luispedro

This comment has been minimized.

Copy link
Contributor Author

commented Dec 3, 2012

At the very least, if you do go ahead and change PyArray_DATA to return char*, it needs to be a documented API change in the release notes with a big warning: this may break your code

@nouiz

This comment has been minimized.

Copy link
Contributor

commented Dec 3, 2012

I'm fine with this PR. I only had problem with PyArray_BYTES, so this shouldn't affect Theano.

Thanks for asking.

@@ -131,7 +131,7 @@
if (PyArray_BASE(new) && PyObject_AsReadBuffer(PyArray_BASE(new),
(const void **)&buf,
&buf_len) >= 0) {
offset = PyArray_DATA(self) - buf;
offset = PyArray_BYTES(self) - buf;

This comment has been minimized.

Copy link
@charris

charris Dec 3, 2012

Member

Heh. Did this raise a compiler error? I just want to make sure we didn't miss any other spots.

This comment has been minimized.

Copy link
@luispedro

luispedro Dec 3, 2012

Author Contributor

The PyArray_DATA raised an error after the change to returning void*, so I changed it to use PyArray_BYTES

This comment has been minimized.

Copy link
@charris

charris Dec 3, 2012

Member

Yes, void pointers can't be used in arithmetic. I just wanted to make sure we fixed all the spots where it could be an issue.

@charris

This comment has been minimized.

Copy link
Member

commented Dec 4, 2012

The Travis build passed, so I'll put this in.

@certik this needs to go after the earlier commit that made the change.

charris added a commit that referenced this pull request Dec 4, 2012
Merge pull request #2780 from luispedro/pyarray-data-voidp
API Make PyArray_DATA return void*

@charris charris merged commit b531ed2 into numpy:master Dec 4, 2012

1 check passed

default The Travis build passed
Details
@njsmith

This comment has been minimized.

Copy link
Member

commented Dec 4, 2012

@certik - this needs a 1.7 backport.

@certik certik referenced this pull request Dec 4, 2012
@certik

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2012

Thanks! Just posted a PR with the backport.

certik added a commit that referenced this pull request Dec 4, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.