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

Adds lint rules to MSVC #247

Closed
wants to merge 2 commits into from
Closed

Adds lint rules to MSVC #247

wants to merge 2 commits into from

Conversation

cugone
Copy link
Contributor

@cugone cugone commented Feb 23, 2020

MSVC version of #246

Adds Code Analysis rules (lint rules) for MSVC by adding checks for all the Core Guidelines. The custom .ruleset file is required to select multiple rule sets to follow.

There are also Microsoft Native Minimum Rules and Microsoft Native Recommended Rules, the Minimum is recommended for all custom rulesets and Recommended is for use with Visual Studio Professional Edition. They are Microsoft-specific so I decided to defer using them.

@cugone cugone requested a review from DanRStevens February 23, 2020 16:29
@cugone
Copy link
Contributor Author

cugone commented Feb 23, 2020

@DanRStevens

The default CA settings and rules are Microsoft Minimum Rules for Debug builds and Disabled for non-Debug builds. I've replaced them with All Core Guidelines for all builds.

Should I turn on Minimum for all builds as well as the Core Guidelines?

@DanRStevens
Copy link
Member

Woah, the builds just took 13-14 minutes per configuration, and produced over 7000 lines of output. That's overwhelming. AppVeyor only shows the most recent 2000 lines, so we're missing most of the output.

I would like to use increased linting support, but I think we're going to have to roll this one out in stages. It's not practical to merge as is.

Maybe we can experiment with enabling just one or two sets of warning in a branch, and focus on fixing those.


I'm uncertain about replacing Microsoft Native Minimum Rules. These might be a different set than the core guidelines. Maybe we'd like to enable both sets of warnings? (Assuming they don't conflict with each other, each requiring something the other will warn about).

@cugone
Copy link
Contributor Author

cugone commented Feb 24, 2020

The warnings for the Core Guidelines are just too numerous to fix in one go. For now I've just enabled Microsoft Native Recommended. It gives good coverage of problem areas and doesn't add much overhead in the way of warning count.

@cugone
Copy link
Contributor Author

cugone commented Mar 30, 2020

Recent warnings fixes and potential additions to the list of warnings to-be-added make this current PR obsolete. Closing to remake.

@cugone cugone closed this Mar 30, 2020
@cugone cugone deleted the enableMSVC_CA branch March 30, 2020 20:20
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