GH-123391: Code generators: Track stack and locals in if statements.
#123397
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ntax error, just a tool limitation.
picnixz
reviewed
Aug 27, 2024
Comment on lines
+426
to
+427
| f"Locals not defined in stack order. " | ||
| f"Expected '{out.name}' is defined before '{undefined}'" |
There was a problem hiding this comment.
Suggested change
| f"Locals not defined in stack order. " | |
| f"Expected '{out.name}' is defined before '{undefined}'" | |
| f"Locals not defined in stack order. " | |
| f"Expected {out.name!r} to be defined before {undefined!r}" |
| break | ||
| undefined = "" | ||
| for out in self.outputs: | ||
| if out.defined: |
There was a problem hiding this comment.
If the first out.defined holds, then we don't really have anything "before" (unless this case is not possible?)
| self.emit(next(tkn_iter)) | ||
| lparen = next(tkn_iter) | ||
| self.emit(lparen) | ||
| assert(lparen.kind == "LPAREN") |
There was a problem hiding this comment.
Suggested change
| assert(lparen.kind == "LPAREN") | |
| assert lparen.kind == "LPAREN" |
| self.emit(next(tkn_iter)) | ||
| lparen = next(tkn_iter) | ||
| self.emit(lparen) | ||
| assert(lparen.kind == "LPAREN") |
There was a problem hiding this comment.
Suggested change
| assert(lparen.kind == "LPAREN") | |
| assert lparen.kind == "LPAREN" |
| self.out.emit(next(tkn_iter)) | ||
| lparen = next(tkn_iter) | ||
| self.emit(lparen) | ||
| assert(lparen.kind == "LPAREN") |
There was a problem hiding this comment.
Suggested change
| assert(lparen.kind == "LPAREN") | |
| assert lparen.kind == "LPAREN" |
Comment on lines
+111
to
+115
| raise analysis_error( | ||
| f"Locals not defined in stack order. " | ||
| f"Expected '{out.name}' is defined before '{undefined}'", | ||
| tkn | ||
| ) |
There was a problem hiding this comment.
Suggested change
| raise analysis_error( | |
| f"Locals not defined in stack order. " | |
| f"Expected '{out.name}' is defined before '{undefined}'", | |
| tkn | |
| ) | |
| raise analysis_error( | |
| f"Locals not defined in stack order. " | |
| f"Expected {out.name!r} to be defined before {undefined!r}", | |
| tkn | |
| ) |
| other.top_offset = self.top_offset.copy() | ||
| other.base_offset = self.base_offset.copy() | ||
| other.variables = [var.copy() for var in self.variables] | ||
| other.defined = set(self.defined) |
There was a problem hiding this comment.
Suggested change
| other.defined = set(self.defined) | |
| other.defined = self.defined.copy() |
| self.out.emit(next(tkn_iter)) | ||
| lparen = next(tkn_iter) | ||
| self.emit(lparen) | ||
| assert(lparen.kind == "LPAREN") |
There was a problem hiding this comment.
Suggested change
| assert(lparen.kind == "LPAREN") | |
| assert lparen.kind == "LPAREN" |
| self.run_cases_test(input, output) | ||
|
|
||
|
|
||
| def test_pep7_condition(self): |
There was a problem hiding this comment.
Maybe test the else branch as well?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds little functionality, but it will enable us to do much more effective stack spilling in the future.
I've deliberately left the spilling functionality for a later PR to make this PR easier to review.
This PR:
Storageclass which combines the state of the stack and the local variables within a micro-op.ifstatements.The resulting generated code is basically the same as we have now.
To simplify parsing, the code generator also enforces PEP 7 rules for braces.
Most of the changes in bytecodes.c are a result of this change.