-
Notifications
You must be signed in to change notification settings - Fork 49
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
feature[next]: Extend Single Static Assignment (SSA) pass to support if statements #1250
Conversation
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.
The algorithm looks good but the implementation looks too verbose to me and unnecessarily hard to read. I recommend to simplify the code before approving the Pr.
Ready for another round. |
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.
It looks almost ready to go. Just some comments about missing type annotations and docstrings.
# An empty annotation always applies to the next assignment. | ||
# So we need to use the correct versioning, but also ensure | ||
# we restore the old versioning afterwards, because no assignment | ||
# actually happens. |
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.
Yes, I understood the same core idea but don't really get the details. I think we should understand what the comment really means and rephrase it in a more comprehensible way before merging the PR, otherwise is useless.
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.
ILGTM.
This PR extends the SSA pass (operating on the Python AST) to support if statements such that every variable is assigned only once per branch, e.g. this
is transformed into
Most of the code was written by @BenWeber42 reviewed by me. See #972 for the discussion. The PR is a prerequisite for #1079 and was spun off from there to ease review.