[cfe-dev] Clang Static Analyzer: False Positive Suppression Support

[Devin Coughlin via cfe-dev]

> On Aug 29, 2016, at 10:50 AM, Craig, Ben <ben.craig at codeaurora.org> wrote:
> 
> On 8/29/2016 11:49 AM, Anna Zaks wrote:
>> 
>>> On Aug 29, 2016, at 7:59 AM, Gábor Horváth <xazax.hun at gmail.com <mailto:xazax.hun at gmail.com>> wrote:
>> 
>>> * Using pragmas one can introduce convenience macros, it might also be better to suppress code regions
>>> * It is better to suppress based on bug type or something like that rather than using the name of the checker (which is not intended to leak out)
>>> * It is good idea to have line offsets to avoid too much noise when reading code
>>> * Anything against pragmas?
>> 
>> Another reason pragmas are better than comments is because this is what the compiler uses for suppressions.
> 
> I'm not opposed to pragmas in concept, but I don't like the typical way they are used for compiler warning suppression.  In particular, I strongly dislike the following pattern as the "default" pattern for warning suppression:
> 
> #if __clang_analyzer__
> #pragma static analyzer push
> #pragma static analyzer disable deadcode
> #endif
> // bad code here
> #if __clang_analyzer__
> #pragma static analyzer pop
> #endif
> 
> Things I don't like about that pattern:
> * verbose

I think a certain amount of verbosity is preferable here to steer users away from using it unless it is actually needed. This is a suppression of last resort.

> * requires modifying code before and after the offending line of code
> * usually requires compiler specific guards.
> * if you forget the trailer, code still works, but you lose diagnostics
> * if you don't know the push / pop pattern, you end up losing diagnostics

We can always diagnose this. Well-named macros would also mitigate the problem:

CLANG_ANALYZER_SUPPRESSION_BEGIN()
CLANG_ANALYZER_SUPPRESSION_END()

> I'm fine with that pattern existing in addition to a much more terse and friendly syntax.  I can see the push / pop pattern being useful for blocks of code.  Just don't make that the default.
> 
> Here's a pattern that I'd prefer:
> // Common header...
> #ifdef __clang_analyzer__
> #define CLANG_SUPPRESS_WARNING(category, line_offset) _Pragma(category, line_offset)
> #else
> #define CLANG_SUPPRESS_WARNING(category, line_offset)
> #endif
> // Source file...
> CLANG_SUPPRESS_WARNING(deadcode, +1)
> // bad code here

Any offsets that aren’t +/-1 strike me as very fragile in the face of code evolution. They would make it very easy to have bad merges that lose the suppression. Also: what do you envision the meaning of “line” being within a macro expansion? Would this make it impossible to suppress diagnostics in an expansion itself?

Devin
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20160829/472569d7/attachment.html>