gh-93691: Compiler's code-gen passes location around instead of holding it on the global compiler state #98001
Conversation
… Assert they are the same as on the compiler state, except a few places where the location is currently wrong/random.
iritkatriel
added
the
🔨 test-with-buildbots
|
🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit c7764ce 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
bedevere-bot
removed
the
🔨 test-with-buildbots
|
Going to retrigger buildbots after merging the refleak fix. |
JelleZijlstra
added
the
🔨 test-with-buildbots
|
🤖 New build scheduled with the buildbot fleet by @JelleZijlstra for commit f8d061b 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
bedevere-bot
removed
the
🔨 test-with-buildbots
|
I notice that some function take a location, and others take a pointer to a location. Why? |
See above (#98001 (comment)).
Yeah could do. |
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.
WIndows warning. Use Py_ssize_t instead of int.
There was a problem hiding this comment.
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.
| @@ -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.
There's a few cases of this pattern:
location loc = LOC(s);
ADDOP...(..., loc, ...);Any reason not to shorten this to:
ADDOP...(..., LOC(S), ...);?
There was a problem hiding this comment.
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!
|
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! |
iritkatriel
added
the
🔨 test-with-buildbots
|
🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit 5418bb5 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
bedevere-bot
removed
the
🔨 test-with-buildbots
* 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) ...
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.