SSA: Replace the Guards interface in the SSA data flow integration. #18934
Conversation
There was a problem hiding this comment.
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Tip: If you use Visual Studio Code, you can request a review from Copilot before you push from the "Source Control" tab. Learn more
github-actions
bot
added
C#
JS
Java
Ruby
Rust
|
Dca is completely uneventful. |
hvitved
left a comment
hvitved
left a comment
There was a problem hiding this comment.
One minor suggestion, otherwise LGTM.
| * Only intended for internal use. | ||
| */ | ||
| class DefinitionExt extends Impl::DefinitionExt { | ||
| deprecated class DefinitionExt extends Impl::DefinitionExt { |
There was a problem hiding this comment.
Since these are inside an internal folder, and has the Only intended for internal use comment, it should be fine to simply delete.
There was a problem hiding this comment.
They're actually used in a couple of deprecated predicates that ultimately end up in the public getALastRead member predicate on Definition in the outwards-facing SSA.qll, so it's less clear cut. That's why I left them in for now.
There was a problem hiding this comment.
We can make it private, though, and possibly push in enough casts from DefinitionExt to Definition that we might be able to eliminate it. Let me check.
There was a problem hiding this comment.
Right. All the uses of DefinitionExt here were actually in contexts for which we could push a context with a cast to Definition. So I'll be able to delete this. Commit incoming.
2 participants
One small cleanup commit for Ruby, and then the primary commit, which updates the Guards-related part of the SSA data flow integration module interface. This should be equivalent, but removes the implicit assumption that guards coincide with the CFG node where the branching happens, this is e.g. false if we want to support indirect guards through a local boolean variable in the SSA interface.