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
Patch CPython to use a type reflection trampoline if possible #3964
Conversation
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.
Thanks, @hoodmane, it looks generally good to me, Let's see what CPython devs respond in the upstream PR.
Personally, I am okay with merging this even if the upstream PR is rejected for some reason, so this does not block other JSPI related features.
+ case 3: | ||
+ return ((three_arg)func)(self, args, kw); | ||
+ default: | ||
+ PyErr_SetString(PyExc_SystemError, "Handler takes too many arguments"); |
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.
What happens if a function falls into here? Is this a "should not happen" condition?
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.
Yes. Maybe we could fail more dramatically to make it more obvious. But I have never seen a function in the wild which takes too many arguments. The issue is always with functions that take too few arguments because they don't care about some of them.
Looks like they are likely willing to merge it, I got an approval. I'm actually pretty surprised that Brett rejected your |
Yeah, I think I'll try it again when they include ctypes module in their WASM build. |
This is a replacement for pyodide#3954 and pyodide#3955 structured as an upstream patch
Okay I'm going to merge this since I think the upstream PR is ready. Hopefully upstream will merge this soon too. |
This is a replacement for #3954 and #3955 structured as an upstream patch.
CF upstream PR:
python/cpython#106219