Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.3k
gh-93691: Compiler's code-gen passes location around instead of holding it on the global compiler state#98001
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
… Assert they are the same as on the compiler state, except a few places where the location is currently wrong/random.
bedevere-bot commentedOct 6, 2022
🤖 New build scheduled with the buildbot fleet by@iritkatriel for commitc7764ce 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
Going to retrigger buildbots after merging the refleak fix. |
bedevere-bot commentedOct 7, 2022
🤖 New build scheduled with the buildbot fleet by@JelleZijlstra for commitf8d061b 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
I notice that some function take a location, and others take a pointer to a location. Why? |
See above (#98001 (comment)).
Yeah could do. |
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.
Nice improvement.
A few comments, but no major issues.
Python/compile.c Outdated
| static location | ||
| last_location_in_body(asdl_stmt_seq *stmts) | ||
| { | ||
| for (int i = asdl_seq_LEN(stmts) - 1; i >= 0; i++) { |
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.
WIndows warning. UsePy_ssize_t instead ofint.
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.
Why is this? There are a few other for loops with int variable in this file, I don't know if they need to change too.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| @@ -4015,8 +4066,10 @@ compiler_visit_stmt(struct compiler *c, stmt_ty s) | |||
| n++; | |||
| } | |||
| } | |||
| ADDOP_I(c, RAISE_VARARGS, (int)n); | |||
| location loc = LOC(s); | |||
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.
There's a few cases of this pattern:
locationloc=LOC(s);ADDOP...(...,loc, ...);
Any reason not to shorten this to:
ADDOP...(...,LOC(S), ...);
?
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.
I'll land this and do this and other cleanup in followup PRs. (There are still the SET_LOC and UNSET_LOC to remove - they do nothing now).
Thanks for the review!
bedevere-bot commentedOct 10, 2022
When you're done making the requested changes, leave the comment: And if you don't make the requested changes, you will be put in the comfy chair! |
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
bedevere-bot commentedOct 17, 2022
🤖 New build scheduled with the buildbot fleet by@iritkatriel for commit5418bb5 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
* main: (31 commits)pythongh-95913: Move subinterpreter exper removal to 3.11 WhatsNew (pythonGH-98345)pythongh-95914: Add What's New item describing PEP 670 changes (python#98315) Remove unused arrange_output_buffer function from zlibmodule.c. (pythonGH-98358)pythongh-98174: Handle EPROTOTYPE under macOS in test_sendfile_fallback_close_peer_in_the_middle_of_receiving (python#98316)pythonGH-98327: Reduce scope of catch_warnings() in _make_subprocess_transport (python#98333)pythongh-93691: Compiler's code-gen passes location around instead of holding it on the global compiler state (pythonGH-98001)pythongh-97669: Create Tools/build/ directory (python#97963)pythongh-95534: Improve gzip reading speed by 10% (python#97664)pythongh-95913: Forward-port int/str security change to 3.11 What's New in main (python#98344)pythonGH-91415: Mention alphabetical sort ordering in the Sorting HOWTO (pythonGH-98336)pythongh-97930: Merge with importlib_resources 5.9 (pythonGH-97929)pythongh-85525: Remove extra row in doc (python#98337)pythongh-85299: Add note warning about entry point guard for asyncio example (python#93457)pythongh-97527: IDLE - fix buggy macosx patch (python#98313)pythongh-98307: Add docstring and documentation for SysLogHandler.createSocket (pythonGH-98319)pythongh-94808: Cover `PyFunction_GetCode`, `PyFunction_GetGlobals`, `PyFunction_GetModule` (python#98158)pythonGH-94597: Deprecate child watcher getters and setters (python#98215)pythongh-98254: Include stdlib module names in error messages for NameErrors (python#98255) Improve speed. Reduce auxiliary memory to 16.6% of the main array. (pythonGH-98294) [doc] Update logging cookbook with an example of custom handling of levels. (pythonGH-98290) ...
Uh oh!
There was an error while loading.Please reload this page.
This implements#93691 (comment).
The line numbers are almost identical to those we calculated before (see the individual commits if you're interested in the process I followed). There were a few place (around annotations, for instance) where the line number was not set so it remained whatever it was before. I didn't bother going through hoops to make this backwards compatible because it's obviously wrong. I just take the line number from the current AST node.
In a few places (like pattern matching) I pass around a pointer to the location which can be updated deep in the recursion. This is not how it should be, but it's how it was until now (the global was updated in the recursion and impacted further nodes). So I used pointers to emulate this and get similar results, but my intention is to get rid of these pointers in the future, and just take locations from the relevant, current, AST node.