Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 33.9k
gh-93649: Split vectorcall testing from _testcapimodule.c#94549
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
encukou commented Jul 4, 2022 • edited by miss-islington
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by miss-islington
Uh oh!
There was an error while loading. Please reload this page.
The _testcapimodule.c file is getting too large to work with effectively. Vectorcall tests aren't the biggest issue -- it's just an area I want to work on next, so I started there. It does make it clear that MethodDescriptor2 is related to testing vectorcall, which wasn't clear before (the /* Test PEP 590 */ section had an ambiguous end). This PR lays out a general structure of how tests can be split up, with more splitting to come later if the structure is OK.
erlend-aasland left a comment
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.
This is IMO a nice improvement. Thanks!
You might want to add Modules/_testcapi/testdcapimodule_parts.h to MODULE__TESTCAPI_DEPS in Makefile.pre.in.
Uh oh!
There was an error while loading. Please reload this page.
vstinner commented Jul 5, 2022
I suggest shorter file names:
Would it be possible to move |
vstinner commented Jul 5, 2022
Yes, I like the idea of splitting the long _testcapimodule.c into multiple shorter C files. |
encukou commented Jul 8, 2022
In a different PR, maybe. There are a few more files to go on that ride, and I'd like to limit this PR to one change. |
vstinner left a comment
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.
While _testcapimodule.c is built as a single C extension, I'm not convinced by the purpose of _testcapi/parts.h which only declares a single function. But it's more explicit for you, go for it.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Victor Stinner <vstinner@python.org>
encukou commented Jul 8, 2022
Thanks! |
encukou commented Jul 8, 2022
_testcapi/parts.h should list the init functions from all the parts. There'll be more :) |
miss-islington commented Jul 8, 2022
Status check is done, and it's a success ✅ . |
tiran commented Jul 8, 2022
The PR broke WASM build builts: https://buildbot.python.org/all/#/builders/1050/builds/86 gh-94695 should fix them |
The
_testcapimodule.cfile is getting too large to work with effectively.This PR lays out a general structure of how tests can be split up, with more splitting to come later if the structure is OK.
Vectorcall tests aren't the biggest issue -- it's just an area I want to work on next, so I'm starting here.
An issue specific to vectorcall tests is that it wasn't clear that e.g.
MethodDescriptor2is related to testing vectorcall: the/* Test PEP 590 */section had an ambiguous end. Separate file should make things like this much clearer.OTOH, for some pieces it might not be clear where they should be -- I left
meth_fastcallwith tests of the other calling conventions. IMO, even with the ambiguity it's still worth it to split the huge file up.I'm not sure about the buildsystem changes, hopefully CI will tell me what's wrong.
@vstinner, @markshannon: Do you think this is a good idea?
Automerge-Triggered-By: GH:encukou