-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rust: Implement UnusedVariable.ql
#17642
Conversation
d7c304c
to
8634dd5
Compare
8634dd5
to
fb9ec24
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
A few questions and optional suggestions.
/** Holds if `e` occurs in the LHS of a (compound) assignment. */ | ||
private predicate assignLhs(Expr e) { | ||
exists(BinaryExpr be | | ||
be.getOperatorName().regexpMatch(".*=") and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel strongly about this one, but I believe matches
is still preferred over regexpMatch
where possible???
be.getOperatorName().regexpMatch(".*=") and | |
be.getOperatorName().matches("%=") and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect matches
to do the same thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, there was a discussion a long while back about whether the optimiser is more aware of matches
, I can't remember the answer. Anyway, up to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
I believe this is our first working problem query! ✨
Since this query is based entirely on syntax, we can implement it already now without SSA.