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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
