Ericsson / codechecker

CodeChecker is an analyzer tooling, defect database and viewer extension for static and dynamic analyzer tools.
https://codechecker.readthedocs.io
Apache License 2.0
2.27k stars 383 forks source link

Allow suppressing a checker on a source file level #2339

Open whisperity opened 5 years ago

whisperity commented 5 years ago

Currently, the // codechecker_suppress mechanics only allow suppressing a report at a particular location. Sometimes, multiple reports of the almost exact same kind, for the exact same reason, can be present multiple times in the same file.

One striking example might be GoogleTest's way of initialising the test type and code via a macro.

See this case snippet: `cert-err58-cpp` result due to GoogleTest macro doing something weird

I'm not sure if suppressing all these reports via a line above TEST(...) is possible, but even if it is, it'd be really annoying to keep copying these lines.

A better solution seems to be to allow suppressing a report for the entire file. Granular scripts which call different CodeChecker analyze calls for parts of the project could be a solution here, but that would be immensely hard to maintain.

The above example is from JetBrains' CLion which has a built-in Clang-Tidy invoker and report shower. Clicking on More options... shows a button where a lot of configuration shows up, one of which is labelled Disable "cert-err58-cpp" for file. CLion automatically reformats the file to add a few #pragma lines and configuration which are not ever picked up by the compiler during compilation, but is either "parsed" via the IDE, or by Clang-Tidy itself to not show this particular warning.

(I'm leaning more on the "parsed by IDE" side, as calling Clang-Tidy on a file with these extra pragmas in still results in the report shown when viewed via CodeChecker.)

Note TEST not being underlined anymore:

The particular issue is not reported anymore.

Perhaps // codechecker_suppress could be extended so that it may appear somewhere in a file, marking that reports that end in the said file should all be considered ignored (with optional comment, etc. as the usual feature set.) For example, I'd be happy to preamble all my test files (could even set an IDE template so when I create a new C++ Test source file it is inserted automatically) with this:

#pragma clang diagnostic push
#pragma ide diagnostic ignored "cert-err58-cpp"

// codechecker ignored [cert-err58-cpp] {sourcefile}
// Test code, if there's not enough space to allocate the
// strings needed to test, just bail out.

/* ... TEMPLATE: Put real test code here. ... */

#pragma clang diagnostic pop

Another idea that could be considered for suppressions is how Clang Format works: you can put // clang-format off // clang-format on markers in the code, and between the off and on part, the formatting does not touch your code, even if it is badly formatted. Similar could be done with suppressions, if one might wants to ignore an entire code block – however, this is not appropriate here: I only want to not care about the static initialisation yadda for the test class bodies, I'd still be happy to see other kinds of issues that might be present in the test codes I've written.

ebarcsa commented 3 years ago

Hi, I support this request as it would fix use cases when legacy / 3rd pty library is used and even if there was chance to refactor, it would take too much time, compared to original commit, it would be easier to whitelist for time, a single or multiple checks. So the outcome now usually to disable that check for entire project which is opposite of what we actually want.