Skip to content

Conversation

@calumgrant
Copy link
Contributor

@calumgrant calumgrant commented Dec 3, 2024

Commit-by-commit review encouraged. It's a fairly small fix that removes a grand total of one FP on DCA.

Pull Request checklist

All query authors

Internal query authors only

  • Autofixes generated based on these changes are valid, only needed if this PR makes significant changes to .ql, .qll, or .qhelp files. See the documentation (internal access required).
  • Changes are validated at scale (internal access required).
  • Adding a new query? Consider also adding the query to autofix.

@github-actions github-actions bot added the C++ label Dec 3, 2024
@calumgrant calumgrant marked this pull request as ready for review December 4, 2024 11:46
@calumgrant calumgrant requested a review from a team as a code owner December 4, 2024 11:46

// GOOD [FALSE POSITIVE]
printf("%d%d"
UNDEFINED_MACRO,
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this is an FP, because it neither occurs on the line with the literal, not occurs in any of the arguments?

Copy link
Contributor Author

@calumgrant calumgrant Dec 4, 2024

Choose a reason for hiding this comment

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

Yes, so this is one of the FPs we saw on DCA that the current query or fixed query fails to catch. The problem is that the syntax error occurs on line 16, whereas the literal and function call are on line 15, and there is no obvious way to associate the two.

There is no ErrorExpr in the function call, and the extent of the function call is only line 15. So right now, I can't think of a good way to detect this case.

It looks like the frontend's syntax error recovery means dropping all of the expressions until the next ), which is pretty reasonable.

Copy link
Contributor

@jketema jketema left a comment

Choose a reason for hiding this comment

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

Needs a change note, otherwise, LGTM.

Copy link
Contributor

@jketema jketema left a comment

Choose a reason for hiding this comment

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

Please remove the commit that changes the date on the change note. This is going to cause a merge conflict with the ongoing 2.20.0 release, which deletes the file.

@calumgrant calumgrant force-pushed the calumgrant/bmn/wrong-number-format-arguments2 branch from 26eaba7 to 2da3d36 Compare December 6, 2024 09:37
@calumgrant calumgrant merged commit defa4cc into main Dec 6, 2024
15 checks passed
@calumgrant calumgrant deleted the calumgrant/bmn/wrong-number-format-arguments2 branch December 6, 2024 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants