C#: Nested field flow#1805
Conversation
jbj
left a comment
jbj
left a comment
There was a problem hiding this comment.
LGTM, although I didn't attempt to understand the details of the Assignable library.
It would have been easier to review if the autoformat changes had been a separate commit from the functionality changes.
| pragma[noinline] | ||
| private ControlFlow::Node getAnAdjacentReadSameVar() { | ||
| Ssa::Internal::adjacentReadPairSameVar(this.getAControlFlowNode(), result) | ||
| Ssa::Internal::adjacentReadPairSameVar(_, this.getAControlFlowNode(), result) |
There was a problem hiding this comment.
I don't know how performance-sensitive this code is, but I'd usually add an extra parameter on the left instead of on the right to avoid a SCAN in callers that pass _.
The opposite might be optimal for the boolean simple, parameter: if it's on the left, then it should be quick in the caller that passes true, and it might (I haven't tested) be fast for other callers too if they pass any(boolean b | b = false or b = true) instead of _.
There was a problem hiding this comment.
I will run a dist-compare to check performance.
calumgrant
left a comment
calumgrant
left a comment
There was a problem hiding this comment.
This is really excellent! A couple of small comments.
| nodeTo.asExprAtNode(cfn) = def.getAFirstReadAtNode(cfn) | ||
| def = nodeFrom.(SsaDefinitionNode).getDefinition() and | ||
| nodeTo.asExprAtNode(cfn) = def.getAFirstReadAtNode(cfn) and | ||
| if usesInstanceField(def) then simple = false else simple = true |
There was a problem hiding this comment.
You could extract if usesInstanceField(def) then simple = false else simple = true into a predicate somewhere, as this repeats 4 times.
| cached | ||
| predicate simpleLocalFlowStep(Node nodeFrom, Node nodeTo) { | ||
| any(LocalFlow::LocalExprStepConfiguration x).hasNodePath(nodeFrom, nodeTo) | ||
| predicate localFlowStepImpl(Node nodeFrom, Node nodeTo, boolean simple) { |
There was a problem hiding this comment.
I think that "simple" needs to be documented and/or renamed, as it's not clear what "simple" means in this context.
|
Here is the dist-compare report: https://git.semmle.com/gist/tom/457c80d20a207bb057827f7d77da6612 (internal link). |
|
Updated dist-compare report: https://git.semmle.com/gist/tom/457c80d20a207bb057827f7d77da6612#file-report2-md |
calumgrant
left a comment
calumgrant
left a comment
There was a problem hiding this comment.
I think this is much clearer.
3 participants
This is the equivalent of @aschackmull's #1766 for Java. Additionally, I have disabled field-based SSA for global data flow (made possible by @jbj's #1780), to avoid getting multiple paths for the same result. This has the extra side-effect that data flow sources, which are field accesses, will not have spurious use-use steps as in this test.