[PATCH] D52984: [analyzer] Checker reviewer's checklist
Artem Dergachev via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Nov 9 14:47:15 PST 2018
NoQ added a comment.
Thx!! Tiny nits.
Comment at: www/analyzer/checker_dev_manual.html:720
+<li>User facing documentation is important for adoption! Make sure the check list updated
+ at the homepage of the analyzer. Also ensure that the description is good quality in
> ensure the description is clear even to non-developers
> to non-developers
Good luck explaining use-after-move to my grandma :)
Like, i mean, probably to non-analyzer-developers?
Comment at: www/analyzer/checker_dev_manual.html:722-723
+<li>Introduce <tt>BugReporterVisitor</tt>s to emit additional notes that better explain the found
+ bug to the user. There are some existing visitors that might be useful for your check,
+ e.g. <tt>trackNullOrUndefValue</tt>. For example, SimpleStreamChecker should highlight
...that explain the warning to the user better.
Comment at: www/analyzer/checker_dev_manual.html:728
+ <tt>checkDeadSymbols</tt>callback to clean the state up.</li>
+<li>The check should handle correctly if a tracked symbol is passed to a function that is
+ unknown to the analyzer. <tt>checkPointerEscape</tt> callback could help you handle
...should correctly handle...
Or: ...should conservatively assume that the program is correct when...
Comment at: www/analyzer/checker_dev_manual.html:744
+ <li><tt>CallEvent::getOriginExpr</tt> is nullable - for example, it returns null for an
+ automatic destructor of a variable. The same applies to all values generated while the
+ call was modeled, eg. <tt>SymbolConjured::getStmt</tt> is nullable.</li>
Not necessarily all values (if the call is inlined, it may actually happen that all values are concrete), but definitely some values.
Comment at: www/analyzer/checker_dev_manual.html:758
+ e.g. for destructors. You could use <tt>NamedDecl::getNameAsString</tt> for those cases.
+ Note that, this method is much slower and should be used sparringly, e.g. only when generating reports
+ but not during analysis.</li>
I suspect that the comma after 'that' is unnecessary.
Comment at: www/analyzer/checker_dev_manual.html:763-764
+ <li>A <tt>BugReporterVisitor</tt> that matches the AST to decide when to emit note instead of
+ examining/diffing the states.</li>
+ <li>In <tt>State->getSVal(Region)</tt>, <tt>Region</tt> is not necessarily a <tt>TypedValueRegion</tt>
Hmm. Because we're actively bashing the developer in this section, i think we should be careful about being clear what's wrong and what's right and why, while keeping every bullet self-contained (so that it wasn't taken out of the context, i.e. "Bad things: 1. X, 2. ..." shouldn't be accidentally understood as "X").
Patterns that you should most likely avoid even if they're not technically wrong:
* `BugReporterVisitor` should most likely not match the AST of the current program point to decide when to emit a note. It is much easier to determine that by observing changes in the program state.
Comment at: www/analyzer/checker_dev_manual.html:777
+ For modeling arithmetic/bitwise/comparison operations, <tt>SValBuilder</tt> should be used.</li>
+ <li>Custom <tt>ProgramPointTags</tt> are created within the checker. There is usually
+ no good reason for a checker to chain multiple nodes together, because checkers aren't worklists.</li>
Comment at: www/analyzer/checker_dev_manual.html:781
+<li>Checkers are encouraged to actively participate in the analysis by sharing
+ its knowledge about the program state with the rest of the analyzer,
+ but they should not be disrupting the analysis unnecessarily:</li>
Comment at: www/analyzer/checker_dev_manual.html:789
+ paths needs to be dropped entirely. For example, it is fine to eagerly split
+ paths while modeling <tt>isalpha(x)</tt> as long as <tt>xi</tt> is constrained accordingly on
+ each path. At the same time, it is not a good idea to split paths over the
Comment at: www/analyzer/checker_dev_manual.html:809-812
+<li>Is <tt>-analyzer-checker=core</tt> included in all test <tt>RUN:</tt> lines? It was never supported
+ to run the analyzer with the core checks disabled. It might cause unexpected behavior and
+ crashes. You should do all your testing with the core checks enabled.</li>
Let's move this bullet to the top? It's kinda lonely here after all those huge sections with tons of sub-bullets.
More information about the cfe-commits