search

GabrielDosReis / ipr

Compiler-neutral Internal Program Representation for C++
BSD 3-Clause "New" or "Revised" License
222 stars 23 forks source link

Enable rules for the Microsoft C++ Analysis action #185

Closed d-winsor closed 2 years ago

d-winsor commented 3 years ago

Add analysis ruleset to be used by the Microsoft C++ Code Analysis action. Warnings should be visible in the PR so this can be used to determine the scope of rules that will be useful for the IPR.

d-winsor commented 3 years ago

I have filtered some warnings in initial ruleset based on what I personally think is not suited for the IPR. These changes can be seen in the ruleset and am happy to turn back on if deemed useful. They focus around noexcept, manual memory management, virtual dtors and GSL features.

For the remaining warnings I went through the results and applied fixes to most of the warnings. Most of the fixes are for modernization and/or reliability. They provide good context for whether we want to disable any additional warnings so any feedback on the changes is appreciated.

One item left to complete before merge is to enable compiler warnings (not just static analysis) and filtering for the test folder which both require updates to the action itself (coming very soon).

d-winsor commented 2 years ago

I am unsure about the changes of auto to const auto for local variables that are not modified

This is enforcement of C++ Core Guidelines: Con.1

Can I ask what you are unsure about? Is it the matter of what the const applies to (pointer vs pointee) or do you consider in redundant?

GabrielDosReis commented 2 years ago

I am unsure about the changes of auto to const auto for local variables that are not modified

This is enforcement of C++ Core Guidelines: Con.1

Can I ask what you are unsure about? Is it the matter of what the const applies to (pointer vs pointee) or do you consider in redundant?

I am unsure I would like to see that particular rule enforced or have changes based on that particular rule in the codebase for now -- and gsl::suppress would be a very heavy handed way to disable that.

Part of it is that a blind constification of local variables that are not "manifestly" changed can have subtle effects on codegen: a const can prevent an efficient move in a return statement and favor an inefficient copy. And I also do not want people to have to wonder "hey, is this one where I need const or not; is the variable being return, etc." See this repro for illustration of the subtlety: https://godbolt.org/z/oTcK49EY3