Skip to content

fix codeql warnings#3489

Merged
StephanTLavavej merged 2 commits intomicrosoft:mainfrom
strega-nil:codeql-bugs
Feb 23, 2023
Merged

fix codeql warnings#3489
StephanTLavavej merged 2 commits intomicrosoft:mainfrom
strega-nil:codeql-bugs

Conversation

@strega-nil-ms
Copy link
Contributor

@strega-nil-ms strega-nil-ms commented Feb 21, 2023 •
edited
Loading

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

strega-nil-ms commented Feb 21, 2023

@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.

CaseyCarter
CaseyCarter approved these changes Feb 21, 2023
View reviewed changes
Copy link
Contributor

@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

strega-nil-ms commented Feb 21, 2023

@CaseyCarter fully agreed

@StephanTLavavej
Copy link
Member

StephanTLavavej commented Feb 22, 2023

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

StephanTLavavej commented Feb 22, 2023

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

StephanTLavavej commented Feb 22, 2023

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

StephanTLavavej commented Feb 23, 2023

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

@CaseyCarter
Copy link
Contributor

CaseyCarter commented Feb 23, 2023

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

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

@StephanTLavavej StephanTLavavej mentioned this pull request Mar 22, 2023
Merged
@StephanTLavavej StephanTLavavej mentioned this pull request Sep 26, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

+1 more reviewer

@CaseyCarter CaseyCarter CaseyCarter approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@StephanTLavavej StephanTLavavej

Labels

enhancement Something can be improved

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@strega-nil-ms @StephanTLavavej @CaseyCarter @strega-nil