C#: Adopt shared CFG construction library from shared controlflow pack
#13595
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
csharp/ql/lib/semmle/code/csharp/controlflow/internal/ControlFlowGraphImpl.qll
Fixed
Show fixed
Hide fixed
hvitved
force-pushed
the
csharp/use-shared-cfg-pack
branch
4 times, most recently
from
d5bf9d5
to
2d431e9
Compare
hvitved
force-pushed
the
csharp/use-shared-cfg-pack
branch
3 times, most recently
from
dd8dee0
to
ad5dc82
Compare
hvitved
force-pushed
the
csharp/use-shared-cfg-pack
branch
from
ad5dc82
to
b69188f
Compare
|
|
||
| /** 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.