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

[Artem Dergachev via Phabricator via cfe-commits]

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
+    <tt>Checkers.td</tt>.</li>
----------------
Szelethus wrote:
> 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
+    <tt>Checkers.td</tt>.</li>
+<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
+<ul>
+  <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").

Eg.,
```
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>
----------------
`<tt>ProgramPointTag</tt>s` (?)


================
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>
----------------
their knowledge


================
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
----------------
`<tt>x</tt>`


================
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>
+</ul>
----------------
Let's move this bullet to the top? It's kinda lonely here after all those huge sections with tons of sub-bullets.


https://reviews.llvm.org/D52984