-
Notifications
You must be signed in to change notification settings - Fork 1.8k
C++: SloppyGlobal: Don't alert on template instantiations, only the template #20232
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
We no longer alert on template instantiations, just the template.
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.
Pull Request Overview
This PR improves the SloppyGlobal CodeQL query to avoid duplicate alerts when checking for poorly named global variables. The change ensures that only template variable declarations are flagged, not their individual instantiations.
Key changes:
- Modified the SloppyGlobal query logic to exclude
VariableTemplateInstantiationinstances - Added test cases with template variables to verify the behavior
- Updated expected results to show only template declarations are reported
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| cpp/ql/src/Best Practices/SloppyGlobal.ql | Added condition to exclude variable template instantiations from alerts |
| cpp/ql/test/query-tests/Best Practices/SloppyGlobal/main.cpp | Added test cases with template variables and their usage |
| cpp/ql/test/query-tests/Best Practices/SloppyGlobal/SloppyGlobal.expected | Updated expected results to include new test cases |
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
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. One nit and two further things that need to happen:
- Could you please start a traced DCA experiment on this?
- Could you please add a change note?
Alerting on instantiations essentially just gives us duplicate alerts