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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: www/analyzer/checker_dev_manual.html:708
 
+<h2 id=links>Checker Reviewer Checklist</h2>
+<ul>
----------------
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?" (?)


================
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>
----------------
Most importantly, "If addTransition() with unspecified predecessor is ever executed after generateNonFatalErrorNode(), is the state split intended?"


================
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>
----------------
You mean like forgetting to add a transition to the second possible state?


================
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>
----------------
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.


https://reviews.llvm.org/D52984