[PATCH] D52984: [analyzer] Checker reviewer's checklist

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.


================
Comment at: www/analyzer/checker_dev_manual.html:708
 
+<h2 id=links>Checker Reviewer Checklist</h2>
+<ul>
----------------
NoQ wrote:
> I think we actually need two separate checklists:
> * for common bugs (careful use of non-fatal error nodes, etc. - stuff we check for on every review) - "Common pitfalls when developing a checker" (?), also "Analyzer-specific coding standards" (?)
> * for out-of-alpha requirements (all boilerplate present, warnings are clear to all users, evaluated on large codebases) - "What makes a good checker?" (?)
Do you think these two lists should be here, or would you prefer two move them to different parts of the document?


================
Comment at: www/analyzer/checker_dev_manual.html:710
+<ul>
+<li>Is there unintended branching in the exploded graph?</li>
+<li>Are there unintended sinks?</li>
----------------
NoQ wrote:
> Most importantly, "If addTransition() with unspecified predecessor is ever executed after generateNonFatalErrorNode(), is the state split intended?"
Is generateNonFatalErrorNode special? Two addTransition calls could have similar unintended effect, though I can imagine that being more rare. 


================
Comment at: www/analyzer/checker_dev_manual.html:711
+<li>Is there unintended branching in the exploded graph?</li>
+<li>Are there unintended sinks?</li>
+<li>Produced reports are easy to understand? Do we need bug visitors? Is there a way to suppress false positives?</li>
----------------
NoQ wrote:
> You mean like forgetting to add a transition to the second possible state?
My intention here is to check if the sinks are justified. Each sink result in coverage loss, so we do not really want to generate them for minor warnings, when we could continue the analysis in a meaningful manner.


================
Comment at: www/analyzer/checker_dev_manual.html:712
+<li>Are there unintended sinks?</li>
+<li>Produced reports are easy to understand? Do we need bug visitors? Is there a way to suppress false positives?</li>
+<li>Have the checker been run on large projects?</li>
----------------
NoQ wrote:
> You mean suppress false positives with a visitor? Interestingly, right now there are almost no checkers that do this. But i suspect that there's no good reason for that, just `trackNullOrUndefValue()` covers most of the needs.
> 
> I believe that this is also the right way to suppress false positives that come from inlined functions in headers. Such as STL, which we currently prevent from being inlined entirely, which not only fixes many false positives, but also introduces some due to information loss. If we instead suppressed positives when we notice that the bug itself or certain interesting path events (potentially checker-specific) occur in headers, we could have had even less false positives and probably some new true positives.
I think this guideline is not followed at the moment, but I have the following vision: For each false positive report, there should be a way to rewrite the source code in a natural way to suppress that report.

E.g.: let's look at malloc checker:
If we take an infeasible path, we could add an assert to help suppress the report (though it is not always trivial, and this is not specific to the checker).
If we have a custom free function, we should be able to mark that function, so no memory leak is reported.
If we have a memory pool like thing, we should be able to communicate this to the checker somehow.

Or in case of conversion checker, we could check for implicit casts, and we could handle explicit casts as intended and do not report them. So false positives could be suppressed by making some implicit casts explicit.


https://reviews.llvm.org/D52984