[cfe-dev] [Patch] Update to Checker Development Manual

[Jordan Rose]

[-cfe-dev, since this is almost ready]

Hey, Sam. This is looking quite good to me; after addressing these comments I'm fine with this. I'll let Anna be the one to commit it, though.

Comments:

>  void ento::registerNewChecker(CheckerManager &mgr) {
> -  mgr.registerChecker<NewChecker>();
> +  mgr.registerChecker<SimpleStreamChecker&gt();
>  }

The name of the function is actually generated from Checkers.td as well, so this should be ento::registerSimpleStreamChecker.

> +<p> Checkers fundamentally extend the class <tt><a
> +href="http://clang.llvm.org/doxygen/classclang_1_1ento_1_1Checker.html">
> +Checker</a></tt>, but they do so in an unusual way. The "class" <tt>Checker</tt>
> +is actually a templated class, and checkers extend a specialization of this
> +class. Unlike the standard <a
> +href="http://en.wikipedia.org/wiki/Curiously_recurring_template_pattern">
> +Curiously Recurring Template Pattern</a>, the specialization being extended uses
> +as its parameter(s) the type of events that the checker is interested in
> +processing. The various types of events that are available are described in the
> +file <a
> +href="http://clang.llvm.org/doxygen/CheckerDocumentation_8cpp_source.html">
> +CheckerDocumentation.cpp</a>

It's strange, but it's not that strange. How about something like this?

"All checkers inherit from the Checker class template. The template parameters describe which events a particular checker is interested in processing. The various types of events..."


> +<p> As an example, consider <tt>SimpleStreamChecker</tt>, a checker that warns
> +about improper use of file stream APIs. This checker needs to take action at the
> +following times:

Manual style: at this point you've already introduced SimpleStreamChecker, so it's weird to say what it does as if we're encountering it for the first time.


> +class SimpleStreamChecker : public Checker<check::PreCall,
> +                                           check::PostCall,
> +                                           check::DeadSysbols,
> +                                           check::PointerEscape> {
> +    public:
> +
> +    void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
> +
> +    void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
> +
> +    void checkDeadSymbols(SymbolReaper &SR, CheckerContext &C) const;
> +
> +    ProgramStateRef checkPointerEscape(ProgramStateRef State,
> +                                       const InvalidatedSymbols &Escaped,
> +                                       const CallEvent *Call,
> +                                       PointerEscapeKind Kind) const;
> +};

LLVM style uses a two-column indent and doesn't indent the access specifier. We don't have to follow LLVM style here, but there's no real reason not to.

Also typo: Sysbols.


> +perform. However, since checkers have no guarantee about the order in which the
> +program will be explored (or that some paths will be explored at all), no state

I know what you're trying to say here, but that parenthetical is a bit awkward. Suggested rephrasing "...order in which the program will be explored, or even that every path will be explored..."


> +<p>All of these macros take as parameters the name to be used for the custom
> +category of state information and the data type(s) to be used for storage. The
> +data type(s) specified will become the parameter type and/or return type of the
> +methods that manipulate the new category of state information. Each of these
> +methods are templated with the name of the custom data type.

It would be nice to mention the name of the convenience typedef you're given here. In the example below, you end up with an alias for ImmutableMap<SymboiRef, int> called ExampleDataTypeTy.


> +When a checker detects a mistake in the analyzed code, it needs a way to report
> +it to the Analyzer so that it can be displayed. This two classes used to
> +construct this report are <tt><a

Awkward grammar here. s/This two/The two/, but also maybe replace capital "Analyzer" with "analyzer core" or "analyzer Core". Within the code, there's not so much of a concept of "the analyzer".


> +If needed, the <tt>addTransition</tt> function can be called with no arguments to return the current
> +<tt>ExplodedNode</tt> without changing the program's state.

This isn't exactly true; addTransition returns a new node without changing the current node's state, but it does return a new node, either for emitting a bug report or just putting your stamp on whatever you are checking. If you use addTransition just to get the current node, it's all to you end up splitting the path.

I'm not exactly sure how I'd rephrase this, but if you really want to just get the initial ExplodedNode, CheckerContext::getPredecessor will do that.

Other than this, things are looking good. Sorry for the long delays in getting this in!
Jordan
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130506/3bae240f/attachment.html>