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

Initialize bool type member variables #329

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

jlee671
Copy link
Contributor

@jlee671 jlee671 commented Jan 9, 2025

What

Fix C26495 warning flagged by Prefast

Why

Stay compliant with the C++ Core Guidelines and improve code safety by initializing bool type member variables

How

Initialize bool type member variables

Testing

Added unit test for default values

@jlee671 jlee671 requested a review from heaths January 9, 2025 21:27
@jlee671
Copy link
Contributor Author

jlee671 commented Jan 9, 2025

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@heaths
Copy link
Member

heaths commented Jan 9, 2025

Could you do a single PR for all prefast issues? This is getting very noisy for me.

@jlee671
Copy link
Contributor Author

jlee671 commented Jan 9, 2025

Will do! I was grouping them by the warnings. We just had another new one come in, I will ping you once I add a fix for that one as well.

@heaths
Copy link
Member

heaths commented Jan 9, 2025

In the past we usually just fixed everything. Otherwise it's so much more work for the dev and reviewer(s).

@jlee671
Copy link
Contributor Author

jlee671 commented Jan 9, 2025

I realized the new bug is actually for vsixbootstrapper repo, so this PR would be the last one in vswhere for now. We have one last binskim bug to fix in this repo, but the estimated time is longer so we would like to get this one reviewed first.

Copy link
Member

@heaths heaths left a comment

Choose a reason for hiding this comment

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

Seems like a dubious warning, but I appreciate your position of having to fix this anyway.

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

Successfully merging this pull request may close these issues.

2 participants