Swift: switch to shared, parameterized CFG library #15219
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
swift/ql/lib/codeql/swift/controlflow/internal/ControlFlowGraphParameter.qll
Fixed
Show fixed
Hide fixed
There was a problem hiding this comment.
Looks good, but there are some odds and ends to finish:
- needs a change note and autoformat.
- fix for the
CfgConsistencytests (it looks like they're trying toimportan old file). - plus see my two comments
abovebelow. - then a DCA run to check for regressions / build confidence.
swift/ql/lib/codeql/swift/controlflow/internal/ControlFlowGraphParameter.qll
Outdated
Show resolved
Hide resolved
| --- | ||
| category: minorAnalysis | ||
| --- | ||
| * The control flow graph library (`codeql.swift.controlflow`) has been transitioned to use the shared implementation from the `codeql/controlflow` qlpack. No result changes are expected due to this change. |
There was a problem hiding this comment.
I'm not sure I would have put a change note here (since no users are expected to notice this change), but I guess it doesn't hurt 😄
There was a problem hiding this comment.
Ah, that's my fault - I asked for a change note because the check was failing, without thinking about whether should have just added the flag to say that one is not required. I agree it doesn't do any harm.
swift/ql/lib/codeql/swift/controlflow/internal/ControlFlowGraphParameter.qll
Outdated
Show resolved
Hide resolved
| @@ -1,3 +1,252 @@ | |||
| #-----| ... = ... | |||
There was a problem hiding this comment.
Awesome! Are these changes all from a9c9170?
rdmarsh2
force-pushed
the
rdmarsh2/swift/parameterized-cfg-library
branch
from
bf31913 to
ec6d8da
Compare
| predicate scopeFirst(CfgScope scope, ControlFlowElement first) { | ||
| scope.(Impl::CfgScope::Range_).entry(first) | ||
| } | ||
| /** Gets the maximum number of splits allowed for a given node. */ |
Check warning
Code scanning / CodeQL
Class QLDoc style.
|
Changes LGTM and I think the DCA run LGTM as well (no result changes, analysis time possibly a speedup). Though I am slightly worried by the join-order badness (v1) result for |
|
That does look like a bad join order that's worth investigating, but it doesn't look like it's actually new - the tuple sum went up by 7802, but the max tuple count for a row is 17186585 both before and after. |
|
Indeed, running this with |
|
This fixes the join: #15348 |
No description provided.