-
Notifications
You must be signed in to change notification settings - Fork 1.7k
C++: Autoformat IR SSA files #1928
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
Conversation
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.
Otherwise LGTM. Did you want Dave to review this or has he already seen it?
cached Instruction getInstructionSuccessor(Instruction instruction, EdgeKind kind) { | ||
if(hasChiNode(_, getOldInstruction(instruction))) | ||
|
||
cached |
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 think the above was intended to be a QLDoc comment attached to the predicate.
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.
Good catch. I'll fix that.
One autoformat omission had also slipped into `DefaultTaintTracking.qll`.
Let's give @dave-bartolomeo a chance to react to this PR in his time zone. Then we can merge tomorrow if there are no objections. |
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 `Chi` instruction consumes the enclosing virtual variable of its use location. | ||
hasChiNode(defLocation, use) | ||
) | ||
else ( | ||
) else ( |
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 find this kind of inconsistent with the usual pattern if the if
, then
, and else
being vertically aligned.
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 agree.
One autoformat omission had also slipped into
DefaultTaintTracking.qll
. When everything is autoformatted after this PR, we can set up a PR check for autoformat.I've searched for all comments that change in the diff, and they look good to me. This will conflict with #1736, but @dave-bartolomeo has given the green light anyway.