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

CLN: Fix return type for initObjToJSON() #5334

Merged
merged 1 commit into from
Oct 26, 2013

Conversation

jtratner
Copy link
Contributor

Make it so that it always returns the same thing as numpy, so that it
matches the right signature whether in or not in the error condition.

Closes #5326.

@ghost ghost assigned jtratner Oct 26, 2013
@jtratner
Copy link
Contributor Author

Here's the in-a-nutshell of why this is necessary: http://stackoverflow.com/questions/10509400/difference-between-pymodinit-func-and-pymodule-create

In Python 2.X the module init function could return void, in 3.X needs to return PyObject* (or NULL). I guess numpy assumes you'd use import_arrays() in a function whose signature is PyMODINIT_FUNC and therefore conveniently includes the appropriate return that you'd need on an error importing numpy multiarray. We weren't actually handling the non-error case correctly - thanks to @toddrme2178 for catching this!

@Komnomnomnom
Copy link
Contributor

Thanks @jtratner for digging into this. Fix looks good.

#if (PY_VERSION_HEX < 0x03000000)
void initObjToJSON(void)
// import_array() compat
#if (PY_VERSION_HEX >= 0x03000000)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

flipped this to match numpy's format for clarity.

jtratner added a commit that referenced this pull request Oct 26, 2013
CLN: Fix return type for initObjToJSON()
@jtratner jtratner merged commit be7c4c0 into pandas-dev:master Oct 26, 2013
@jtratner jtratner deleted the try-fix-for-initobjtojson branch October 26, 2013 14:13
@jreback
Copy link
Contributor

jreback commented Oct 26, 2013

fyi...this builds on windows! so that is good

@jtratner
Copy link
Contributor Author

good, and it ought to build on openSUSE too...

@toddrme2178
Copy link

I can confirm this fixes the problem on openSUSE. Thank you for the prompt
fix. I have backported the patch to the 0.12.0 release.

On Sun, Oct 27, 2013 at 1:41 AM, Jeff Tratner notifications@git.luolix.topwrote:

good, and it ought to build on openSUSE too...


Reply to this email directly or view it on GitHubhttps://github.com//pull/5334#issuecomment-27158343
.

@jtratner
Copy link
Contributor Author

No problem - thanks for reporting this! - it was a subtle error (and it was
helpful to look into the numpy C API further). It also speaks to the
utility of the openSUSE build QC. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

objToJSON nonvoid function does not return a value
4 participants