- Notifications
You must be signed in to change notification settings - Fork3.8k
Fix and normalize function pointer calls for METH_NOARGS#3080
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Thanks for the PR and trying to make pygame better. The only long-term solution I see is to use some checker script in the CI builds. Otherwise those errors will creep in again. |
Starbuck5 commentedMar 17, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I'm 100% spitballing here, but what if one of pygame's base header files undefined or invalidated the actual things you would use to cast functions, forcing people to write the functions without casts. Also, please correct me if I'm wrong, but shouldn't the (PyCFunction) casts be removed now? Things like this:https://github.com/pygame/pygame/blob/main/src_c/math.c#L3292-L3295. This PR doesn't remove function pointer casting fully, just makes it an easier cast for Webassembly? |
indeed, it just try to fix what is completely illegal in web assembly, trying to leave existing code as possible untouched. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Let's do it!
Thanks for working on this pmp-p!
The funny thing is that this actually might be a code quality enhancement, since it gives a consistent way to express the PyObject in the slot isn't and shouldn't actually be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM! Thanks for the PR 👍 😄
As mentioned above, it might be a good idea to automate testing this on CI somehow, but apart from that, this PR is nice and it makes things more consistent!
PS: The Ubuntu fail on this PR is unrelated to this issue, it should already be fixed onmain branch
Uh oh!
There was an error while loading.Please reload this page.
FIXES ( ie when *_null arg is missing in C) are related to Web Assembly port ( most of it currently working ).
some background herehttps://blog.pyodide.org/posts/function-pointer-cast-handling/
the rest is a normalization of METH_NOARGS conventions, *noarg was used only once herehttps://github.com/pmp-p/pygame-wasm/blob/37cc2e82ebba825dc90aedbd7ee620b93931a8ee/src_c/music.c#L447
once done i will be able to add fixes to support better Limited API and full wasm support which must be reviewed more carefully than this one.
As the current changes proposed where made guided by a checker (https://gist.github.com/pmp-p/5641b7edfffba457d3eed875454ab5ca)
concerns#718