Skip to content

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

Merged
merged 2 commits into from
Sep 12, 2019
Merged

Conversation

jbj
Copy link
Contributor

@jbj jbj commented Sep 12, 2019

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.

@jbj jbj added the C++ label Sep 12, 2019
@jbj jbj requested a review from a team as a code owner September 12, 2019 11:27
Copy link
Contributor

@geoffw0 geoffw0 left a 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
Copy link
Contributor

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.

Copy link
Contributor Author

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.

jbj added 2 commits September 12, 2019 15:44
One autoformat omission had also slipped into
`DefaultTaintTracking.qll`.
@jbj
Copy link
Contributor Author

jbj commented Sep 12, 2019

Did you want Dave to review this or has he already seen it?

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.

Copy link
Contributor

@dave-bartolomeo dave-bartolomeo left a 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 (
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree.

@dave-bartolomeo dave-bartolomeo merged commit 9072f62 into github:master Sep 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants