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

MAINT: Fix METH_NOARGS function signatures #20368

Merged
merged 6 commits into from Nov 18, 2021

Conversation

hoodmane
Copy link
Contributor

@hoodmane hoodmane commented Nov 14, 2021

This is a continuation of @joemarshall's work #19058.

I think there should be a bunch of more issues around METH_NOARGS usage that would be nice to fix up as well.
@seberg in #19058 (comment)

This fixes all of the incorrect METH_NOARGS signatures that I managed to track down.
We need this for pyodide/pyodide#1677.

The METH_NOARGS calling convention is required to take a second PyObject* which will always be NULL.
This is a continuation of numpy#19058
@seberg
Copy link
Member

seberg commented Nov 17, 2021

Thanks! Would you be up to making it throughout NPY_UNUSED(args)? Since I think it is a bit more clear what this is. One (or two) of them are also a bit long lines, we try to stay under 80 characters.

But changes look good, just thought a bit more uniform naming will be easier to understand.

@seberg seberg changed the title MAINT Fix METH_NOARGS function signatures in nditer_pywrap.c MAINT: Fix METH_NOARGS function signatures Nov 17, 2021
@hoodmane
Copy link
Contributor Author

hoodmane commented Nov 18, 2021

I don't understand the failing doctests.

@hoodmane
Copy link
Contributor Author

hoodmane commented Nov 18, 2021

Also, if you want me to clean the history I am happy to rebase and squash all the commits.

@seberg
Copy link
Member

seberg commented Nov 18, 2021

We can just squash-merge, curious what is "closure"? (I think I just come form a different angle, I thought "args", because most python functions get args or arg passed in, but here it is always NULL.)

No need to squash, if you don't we will probably just squash-merge.

@hoodmane
Copy link
Contributor Author

hoodmane commented Nov 18, 2021

I think args is sort of the right name for the extra argument of the METH_NOARGS functions. But the argument really doesn't mean anything: it just gets filled in with NULL here:
https://github.com/python/cpython/blob/fc4474e45eecbea8e88095f28c98c5d56438d841/Objects/methodobject.c#L489
I am not really sure why they put that in, it just seems to cause trouble.

The void * argument to getters and setters I think should best be called void *closure.

See here: https://docs.python.org/3/c-api/structures.html?highlight=getset#c.PyGetSetDef

void *closure -- optional function pointer, providing additional data for getter and setter

In the CPython source, sometimes the extra noargs arguments are called args (13 times) but much more often they are called ignored (780 times).

The last argument to getters and setters is variously called closure, context, or ignored in CPython source.

This is more than you wanted to know I'm sure =)

@seberg
Copy link
Member

seberg commented Nov 18, 2021

I am not really sure why they put that in, it just seems to cause trouble.

Well, could be to save some branching. Or just historical, maybe METH_NORARG was added later for convenience...

This is more than you wanted to know I'm sure =)

Hehehe, thanks :). I was just confused why closure has any meaning in this context (but it is also a term I am simply not really used to).

Anyway, thanks! Let me put this in, before I find something else to nitpick about ;).

@seberg seberg merged commit 258ce25 into numpy:main Nov 18, 2021
33 of 37 checks passed
@seberg seberg added the 09 - Backport-Candidate PRs tagged should be backported label Nov 18, 2021
@seberg
Copy link
Member

seberg commented Nov 18, 2021

Marking as backport candidate, since it is harmless and you mentioned you need it for pyodine, so presumably it would be nice to have a released version with it in.

@hoodmane
Copy link
Contributor Author

hoodmane commented Nov 18, 2021

Thanks!

@hoodmane hoodmane deleted the pywrap-METH_NOARGS branch Nov 18, 2021
@charris charris added this to the 1.22.0 release milestone Nov 19, 2021
charris pushed a commit to charris/numpy that referenced this pull request Nov 19, 2021
The METH_NOARGS calling convention is required to take a second PyObject* which will always be NULL.

This is a continuation of numpy#19058
@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label Nov 19, 2021
@charris charris removed this from the 1.22.0 release milestone Nov 19, 2021
@charris
Copy link
Member

charris commented Nov 19, 2021

The circleci failures are legitimate.

@charris
Copy link
Member

charris commented Nov 19, 2021

But I don't see exactly how the CircleCI failure is related to this.

@seberg
Copy link
Member

seberg commented Nov 19, 2021

Hmmm, and only on this PR! Very strange...

@charris
Copy link
Member

charris commented Nov 19, 2021

Also in the backport.

@hoodmane
Copy link
Contributor Author

hoodmane commented Nov 19, 2021

After the most recent commit, these tests are passing on main again. At a glance it is very unclear why the most recent commit would have any effect on these tests either.

charris added a commit that referenced this pull request Nov 21, 2021
MAINT: Fix METH_NOARGS function signatures (#20368)
charris pushed a commit to charris/numpy that referenced this pull request Nov 21, 2021
The METH_NOARGS calling convention is required to take a second PyObject* which will always be NULL.

This is a continuation of numpy#19058
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants