-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Ruby: Implement localMustFlowStep
#14303
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
Conversation
66aa1c9 to
5ebedc7
Compare
5ebedc7 to
faf717b
Compare
faf717b to
9bc395b
Compare
9bc395b to
8d913bd
Compare
8d913bd to
83d3e65
Compare
83d3e65 to
de0deab
Compare
| node2.asExpr() = SsaImpl::getARead(def) | ||
| ) | ||
| or | ||
| node1.asExpr() = node2.asExpr().(CfgNodes::ExprNodes::AssignExprCfgNode).getRhs() |
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.
This looks good to me, I was just wondering if there was overlap between this line and def.(Ssa::WriteDefinition).assigns(node1.asExpr()) above. Both seem to be related to assignments. Similar question about SsaFlow::toParameterNodeImpl and the special handling for (ImplicitBlockArgumentNode).getParameterNode(_).
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.
This looks good to me, I was just wondering if there was overlap between this line and def.(Ssa::WriteDefinition).assigns(node1.asExpr()) above. Both seem to be related to assignments.
The first line is about flow from e to SSA def for x in x = e, and the second line is about flow from e to the whole assignment expression x = e.
Similar question about SsaFlow::toParameterNodeImpl and the special handling for (ImplicitBlockArgumentNode).getParameterNode(_).
This is because implicit block parameters and arguments are not covered by our SSA analysis, otherwise you are right it would be an instance of that.
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 for clarifying.
aibaars
left a comment
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.
LGTM, just had a question.
Now that we track types in data flow, implementing
localMustFlowStepmeans that we will also get type-based call-target pruning, as implemented in the shared library.