<html><head><meta http-equiv="Content-Type" content="text/html charset=iso-8859-1"><meta http-equiv="Content-Type" content="text/html charset=iso-8859-1"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><span style="color: rgb(34, 34, 34); line-height: 19px; text-align: left; background-color: rgb(255, 255, 255); ">Sam, </span></div><div><span style="color: rgb(34, 34, 34); line-height: 19px; text-align: left; background-color: rgb(255, 255, 255); "><br></span></div><div><span style="color: rgb(34, 34, 34); line-height: 19px; text-align: left; background-color: rgb(255, 255, 255); ">This looks very good! I have a couple of minor comments below. I would also like to have the CustomProgram States section move before the commit.</span></div><div><span style="color: rgb(34, 34, 34); line-height: 19px; text-align: left; background-color: rgb(255, 255, 255); "><br></span></div><div><span style="color: rgb(34, 34, 34); line-height: 19px; text-align: left; background-color: rgb(255, 255, 255); ">--- Custom Program States</span></div><span style="color: rgb(34, 34, 34); line-height: 19px; text-align: left; background-color: rgb(255, 255, 255); ">-The data type(s) specified will also become the parameter and/or return type </span><div><div style="text-align: left;"><span style="color: rgb(34, 34, 34); line-height: 19px; background-color: rgb(255, 255, 255); ">+The data type(s) specified will also become the parameter types and/or return type </span></div><div style="text-align: left;"><span style="line-height: 19px; color: rgb(34, 34, 34); ">It would be better to present either a map or a set in the example of register with program state macros as these are the most common (in particular, a map that has a SymbolRef key)</span></div><div style="text-align: left;"><font color="#222222"><span style="line-height: 19px;">Also, "Custom Program States" section should come after "Events Callbacks,..". </span></font></div><div style="text-align: left;"><span style="color: rgb(34, 34, 34); line-height: 19px; background-color: rgb(255, 255, 255); "><br></span></div><div style="text-align: left;"><span style="color: rgb(34, 34, 34); line-height: 19px; background-color: rgb(255, 255, 255); ">--- Idea for a Checker</span></div><div style="text-align: left;"><span style="color: rgb(34, 34, 34); line-height: 19px; background-color: rgb(255, 255, 255); ">We could briefly introduce the SimpleStreamChecker and what it does at the end of Idea for a Checker section. What do you think?</span></div><div style="text-align: left;"><span style="color: rgb(34, 34, 34); line-height: 19px; background-color: rgb(255, 255, 255); "><br></span></div><div style="text-align: left;"><span style="color: rgb(34, 34, 34); line-height: 19px; background-color: rgb(255, 255, 255); ">--- Events, Callbacks.. section</span></div><div style="text-align: left;"><span style="color: rgb(34, 34, 34); line-height: 19px; background-color: rgb(255, 255, 255); ">-For each event type requested, a corresponding callback function with the name </span><tt style="background-color: rgb(255, 255, 255); ">check<event></tt><span style="color: rgb(34, 34, 34); line-height: 19px; background-color: rgb(255, 255, 255); "> must be defined </span></div><div style="text-align: left;"><span style="color: rgb(34, 34, 34); line-height: 19px; background-color: rgb(255, 255, 255); ">+For each event type requested, a corresponding callback function</span><span style="color: rgb(34, 34, 34); line-height: 19px; background-color: rgb(255, 255, 255); "> must be defined </span></div><div style="text-align: left;"><font color="#222222"><span style="line-height: 19px;">Since some functions don't start with check and "<event>" does not follow the format.. </span></font></div><div style="text-align: left;"><span style="color: rgb(34, 34, 34); line-height: 19px; background-color: rgb(255, 255, 255); "><br></span></div><div style="text-align: left;"><span style="color: rgb(34, 34, 34); line-height: 19px; background-color: rgb(255, 255, 255); ">--- Bug Reports section</span></div><div style="text-align: left;"><span style="color: rgb(34, 34, 34); line-height: 19px; background-color: rgb(255, 255, 255); ">-This two classes used to construct this report are</span></div><div style="text-align: left;"><span style="color: rgb(34, 34, 34); line-height: 19px; background-color: rgb(255, 255, 255); ">+The two classes used to construct this report are</span></div><div style="text-align: left;"><ol style="color: rgb(34, 34, 34); line-height: 19px; "><li style="padding-bottom: 0.5em; ">If the state has been updated, then the <a href="http://clang.llvm.org/doxygen/classclang_1_1ento_1_1CheckerContext.html#a264f48d97809707049689c37aa35af78">CheckerContext::addTransition</a> function will return the <tt style="color: rgb(0, 0, 0); ">ExplodedNode</tt> with the updated state.</li><li style="padding-bottom: 0.5em; ">If the state has not been updated, the <tt style="color: rgb(0, 0, 0); ">ExplodedNode</tt> can be obtained by calling the <tt style="color: rgb(0, 0, 0); ">CheckerContext::addTransition</tt> function with no arguments.</li></ol><div>These two cases are more or less the same. I'd just first, describe the difference between sink and regular node(the paragraph below) and after that give the two options: addTransition and generateSink, which are essentially the same except for generating a transition to sink or a regular node. these would be my 1 & 2 option bullets. (The API names are not consistent :( )</div><div><br></div><div>Thanks!</div><div>Anna.</div><div><br></div></div><div><div>On Apr 21, 2013, at 4:55 PM, Sam Handler <<a href="mailto:samuel.handler+cfedev@gmail.com">samuel.handler+cfedev@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div dir="ltr"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Apr 15, 2013 at 1:28 PM, Anna Zaks <span dir="ltr"><<a href="mailto:ganna@apple.com" target="_blank">ganna@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div style="word-wrap:break-word"><br><div><div><div>On Apr 14, 2013, at 4:11 PM, Sam Handler <<a href="mailto:samuel.handler+cfedev@gmail.com" target="_blank">samuel.handler+cfedev@gmail.com</a>> wrote</div>
</div><div><div><br><blockquote type="cite"><div dir="ltr" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="word-wrap:break-word">
<div><span style="line-height:19px;text-align:left;color:rgb(34,34,34)"></span></div><div style="text-align:left"><span style="line-height:18px"></span></div><div style="text-align:left"><span style="line-height:18px">Also, addTransition always has arguments.</span></div>
</div></blockquote><div><br></div><div>I've changed it to suggest CheckerContext::getState instead of addTransition in cases where there are no state updates and the updated state (as returned by addTransition) in cases where there are state updates.<br>
</div><div> </div></div></div></div></div></blockquote><div><br></div></div><div>That does not seem right. A node consists of a ProgramState AND a ProgramPoint.</div></div><div><br></div><div>However I've realized that, here, my previous review comment is not correct. You CAN call addTransition with no arguments:</div>
<div style="margin:0px;font-family:Menlo"> <span style="color:rgb(79,129,135)">ExplodedNode</span> *addTransition(<span style="color:rgb(79,129,135)">ProgramStateRef</span> State = <span style="color:rgb(39,42,216)">0</span>,</div>
<div style="margin:0px;font-family:Menlo"> <span style="color:rgb(187,44,162)">const</span> <span style="color:rgb(79,129,135)">ProgramPointTag</span> *Tag = <span style="color:rgb(39,42,216)">0</span>) {</div>
<div style="margin:0px;font-family:Menlo"> <span style="color:rgb(187,44,162)">return</span> <span style="color:rgb(49,89,93)">addTransitionImpl</span>(State ? State : <span style="color:rgb(49,89,93)">getState</span>(), <span style="color:rgb(187,44,162)">false</span>, <span style="color:rgb(39,42,216)">0</span>, Tag);</div>
<div style="margin:0px;font-family:Menlo"> }</div><div><br></div><div>In that case, the current ProgramState will be used. However, a new node will be created for the bug report. The node will be different from the previous node - it will be tagged to identify that it has been produced by the current checker. Sorry for the confusion!</div>
</div></div></blockquote><div><br></div><div>A quick survey of the existing checkers indicates that, in cases where a sink node or state transition is not being generated, the most popular way to get an ExplodedNode to pass to BugReport is addTransition with no arguments. I've changed my text to recommend this.<br>
</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="word-wrap:break-word"><div><div><div></div><blockquote type="cite">
<div dir="ltr" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><br><div> - Getting Started<br></div><div> - Static Analyzer overview // Without the details of how states are represented<br></div><div> - Idea for a checker // Also merge the current "AST Visitors" section into this one.<br>
</div></div></div></div></div></blockquote></div>Let's leave "AST Visitors" in the separate section. They are very different from the regular checkers. Someone learning about path sensitive checkers does not need to know about how to write an AST Visitor.</div>
</div></blockquote><div><br></div><div>Ah, I misinterpreted that section. I read "AST Visitor" as Clang's ASTVisitor (which could be used to implement "checks" that do not need path analysis), not as a separate item specific to the analyzer. I now agree that that section should be left in place (and, of course, should be expanded at some point).<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="word-wrap:break-word"><div><div><br><blockquote type="cite">
<div dir="ltr" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> - Checker Registration<br></div><div> - Checker Skeleton<br></div></div></div></div></div></blockquote></div>Maybe rename into Checker Callbacks? That's what the new content seems to be.</div>
</div></blockquote><div><br></div><div>Changed it to "Events, Callbacks, and Checker Class Structure". All three are discussed, and are closely related to each other.<br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div style="word-wrap:break-word"><div></div><div><div>+<p>All of these macros take as parameters the name to be used for the custom</div><div>"All of these macros take two parameters: the name to be used for the custom"</div>
</div></div></blockquote><div><br>REGISTER_MAP_WITH_PROGRAMSTATE takes three parameters, hence the "type(s)".<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div style="word-wrap:break-word"><br><div><div>+ <li>Create a new checker implementation file, for example <tt>./lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp</tt>.</div><div>+ <li>Include the following code (replacing SimpleStreamChecker with the name of the new checker).</div>
<div>This is a bit confusing. We probably do not want people to create "SimpleStreamChecker.cpp" as such a file already exists. Maybe we should just change the name to something else throughout the manual. For example, we could use MySimpleStreamChecker. In that case, we would also not need to tell the reader to change the name, as in bullet #2 above. What do you think?</div>
<div>We could also mention that the sample code used in the manual is based on the SimpleStreamChecker from the presentation.</div></div></div></blockquote><div><br></div><div>I've changed the tone of this section from "this is what you do for a new checker" to "this is what was done for SimpleStreamChecker", which matches the tone of the other sections better. I don't think anyone will have trouble determining what needs to be changed for their own new checkers. However, I'm not sure how well this section reads after this change, please review and give your opinion.<br>
</div><div><br></div><div>Also, would it be a good idea to have a section giving an overview of the SimpleStreamChecker and declaring that it will be the example used throughout the document? This would mean that we could simply say "for SimpleStreamChecker" instead of having phrases like "for SimpleStreamChecker, a checker that warns about improper use of file stream APIs" throughout the document.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="word-wrap:break-word"><div><br></div><div><div>- There are two main decisions you need to make:</div>
<div>- <ul></div><div>- <li> Which events the checker should be tracking. </div><div>- See <a href="<a href="http://clang.llvm.org/doxygen/classento_1_1CheckerDocumentation.html" target="_blank">http://clang.llvm.org/doxygen/classento_1_1CheckerDocumentation.html</a>">CheckerDocumentation</a> </div>
<div>- for the list of available checker callbacks.</li></div><div>- <li> What data you want to store as part of the checker-specific program </div><div>- state. Try to minimize the checker state as much as possible. </li></div>
<div>- </ul></div><div>I think it's very important to give people this high level roadmap, before diving into the example. Maybe this could be moved into a separate section in the very beginning. You can then link the first bullet to Checker Skeleton and the second one to Custom Program States. Maybe we could move this into the Idea for a Checker?</div>
</div></div></blockquote><div><br></div><div>I've reinserted this material (with minor adjustment) in the "Idea for a Checker" section.<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div style="word-wrap:break-word"><div><br></div><div><br></div><div><div><div>+the bug in the code and the program's state when this location is reached. This</div><div>+is in the form of a node in the <tt>ExplodedGraph</tt>.</div>
</div><div><div><br></div><div>"the bug in the code " -> "the bug in the source code "</div><div><div>"and the program's state when this location is reached. This is in the form of a node" -> "and the <tt>ExplodedNode</tt> which encompasses the state of the program at the time the bug is encountered"</div>
</div></div></div></div></blockquote><div><br></div><div>I've rewritten it to emphasize that both the program location and the program state are contained in the ExplodedNode.<br><br><br><br></div><div>Updated and freshly rebased patch attached.<br>
<br></div><div>--Sam<br></div><div><br></div></div></div></div><br clear="all"><br>-- <br>===============================================================================<br>All opinions expressed in posts from this account are entirely my own, and not<br>
those of any present or former employer. Furthermore, I assert that all works<br>contributed to the Clang project (1) were developed using no equipment,<br>supplies, facility or trade secrets of any such employer, (2) were developed<br>
entirely on my own time, and (3) do not result from any work performed for any<br>such employer.<br><br>
</div>
<span><checker_dev_manual.diff></span></blockquote></div><br></div></body></html>