C#: Adopt shared CFG construction library from shared controlflow pack#13595
C#: Adopt shared CFG construction library from shared controlflow pack#13595hvitved merged 1 commit intogithub:mainfrom
controlflow pack#13595Conversation
csharp/ql/lib/semmle/code/csharp/controlflow/internal/ControlFlowGraphImpl.qll
Fixed
Show fixed
Hide fixed
d5bf9d5 to
2d431e9
Compare
dd8dee0 to
ad5dc82
Compare
ad5dc82 to
b69188f
Compare
michaelnebel
left a comment
There was a problem hiding this comment.
Looks plausible/good to me! :-)
Thank you @hvitved !
|
|
||
| /** An AST node. */ | ||
| class AstNode extends Element, TAstNode { | ||
| AstNode() { this = getAChild*(any(@top_level_exprorstmt_parent p | not p instanceof Attribute)) } |
There was a problem hiding this comment.
It looks like the AstNode class is supposed to be superset of ControlFlowElement (there are both implicit and explicit casts from AstNode -> ControlFlowElement), but it doesn't look like the ControlFlowElement class has as a requirement that its elements should be reachable (via the child relation) from a top level statement/expression?
It looks like the AstNode class is equivalent to the (now deleted) Range class in the ControlFlowTree module.
There was a problem hiding this comment.
Correct. (All ControlFlowElements are reachable from a @top_level_exprorstmt_parent; see {expr,stmt}_parent(_top_level) in the DB scheme).
|
|
||
| /** Gets a split for this control flow node, if any. */ | ||
| Split getASplit() { result = splits.getASplit() } | ||
| final Split getASplit() { result = super.getASplit() } |
There was a problem hiding this comment.
Should it be mentioned in a change note that there are some predicates that have been made final (or maybe it is unlikely that anyone would have extended this class)?
There was a problem hiding this comment.
I don't think a change note is needed.
| child in [this.getPattern(), this.getCondition().(ControlFlowElement), this.getBody()] | ||
| abstract private class CaseTree extends ControlFlowTree instanceof Case { | ||
| final override predicate propagatesAbnormal(AstNode child) { | ||
| child in [super.getPattern().(ControlFlowElement), super.getCondition(), super.getBody()] |
There was a problem hiding this comment.
Is it intentional that we now require specifically that the pattern is a ControlFlowElement instead of the condition?
There was a problem hiding this comment.
Its a stylistic change only; I thought it looked nicer to have the cast on the first element in the list.
#13509 follow-up.