Skip to content
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

fix codeql warnings #3489

Merged
merged 2 commits into from
Feb 23, 2023
Merged

fix codeql warnings #3489

merged 2 commits into from
Feb 23, 2023

Conversation

strega-nil-ms
Copy link
Contributor

@strega-nil-ms strega-nil-ms commented Feb 21, 2023

Same as #3478 but not from a microsoft/STL branch.

Link to internal codeql bugs:

I'm still not sure if we should have these, and I'm unsure how to test these changes.

@strega-nil-ms strega-nil-ms requested a review from a team as a code owner February 21, 2023 16:47
@strega-nil-ms
Copy link
Contributor Author

@AlexGuteniev said:

Have you tried more documented and common annotations, instead of some specific to a particular tool (I first time read about)?

that would certainly make me happier, but this is an internal thing.

Copy link
Member

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

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

I'm not enthusiastic about adding arcane comments to our source to suppress static analysis false positives from some unknowable internal process, but if we must do so, I believe these changes are minimally damaging.

@CaseyCarter CaseyCarter added enhancement Something can be improved decision needed We need to choose something before working on this labels Feb 21, 2023
@strega-nil-ms
Copy link
Contributor Author

@CaseyCarter fully agreed

@StephanTLavavej
Copy link
Member

I double-checked all of the control flow - the code is safe, but it's squirrelly enough 🐿️ that I can't really blame CodeQL for warning. A handful of suppressions are acceptable, but I would reconsider if this ended up growing to dozens and dozens.

Thanks for making the changes here as non-invasive as possible.

@StephanTLavavej StephanTLavavej self-assigned this Feb 22, 2023
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

(I will, of course, wait for the decision needed.)

@StephanTLavavej StephanTLavavej removed the decision needed We need to choose something before working on this label Feb 22, 2023
@StephanTLavavej
Copy link
Member

We've reluctantly decided that there's no way around merging this.

@StephanTLavavej StephanTLavavej merged commit 86acfb1 into microsoft:main Feb 23, 2023
@StephanTLavavej
Copy link
Member

Thanks for investigating and fixing these delightful warnings! ⚠️ 🛠️ 😹

@CaseyCarter
Copy link
Member

Thanks for investigating and fixing these delightful warnings! ⚠️ 🛠️ 😹

I think you mean "possibly fixing", since we have no way to verify 🤪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants