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.
|
🤖 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. |
|
Going to retrigger buildbots after merging the refleak fix. |
|
🤖 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. |
|
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! |
|
🤖 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. |
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.