<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class="">Thanks Gábor. This looks great.</div><div class=""><br class=""></div><div class="">One last thing that occurred to me is that we should not create a ModelInjector at all unless the model-path is specified. This allows us to preserve the existing behavior in the analyzer while we continue to evolve this new functionality.</div><div class=""><br class=""></div><div class="">Specifically:</div><div class=""><br class=""></div><div class=""><div class=""> std::unique_ptr<AnalysisASTConsumer></div><div class="">-ento::CreateAnalysisConsumer(const Preprocessor &pp, const std::string &outDir,</div><div class="">- AnalyzerOptionsRef opts,</div><div class="">- ArrayRef<std::string> plugins) {</div><div class="">+ento::CreateAnalysisConsumer(CompilerInstance &CI) {</div><div class=""> // Disable the effects of '-Werror' when using the AnalysisConsumer.</div><div class="">- pp.getDiagnostics().setWarningsAsErrors(false);</div><div class="">+ CI.getPreprocessor().getDiagnostics().setWarningsAsErrors(false);</div><div class=""> </div><div class="">- return llvm::make_unique<AnalysisConsumer>(pp, outDir, opts, plugins);</div><div class="">+ return llvm::make_unique<AnalysisConsumer>(</div><div class="">+ CI.getPreprocessor(), CI.getFrontendOpts().OutputFile,</div><div class="">+ CI.getAnalyzerOpts(), CI.getFrontendOpts().Plugins,</div><div class="">+ new ModelInjector(CI));</div><div class=""> }</div><div class=""> </div></div><div class=""><br class=""></div><div class="">We can query 'opts' to see if model-path is empty; if it is we can pass nullptr instead of 'new ModelInjector(CI)'.</div><br class=""><div><blockquote type="cite" class=""><div class="">On Aug 12, 2014, at 1:22 AM, Gábor Horváth <<a href="mailto:xazax.hun@gmail.com" class="">xazax.hun@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" style="font-family: Helvetica; font-size: 14px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">Hi Ted,<br class=""><br class="">Thank you for the review, I have altered the patch accordingly, and also added the patch to follow up the API change in clang tidy.<br class=""><div class="gmail_extra"><br class=""><br class=""><div class="gmail_quote">On 12 August 2014 08:44, Ted Kremenek<span class="Apple-converted-space"> </span><span dir="ltr" class=""><<a href="mailto:kremenek@apple.com" target="_blank" class="">kremenek@apple.com</a>></span><span class="Apple-converted-space"> </span>wrote:<br class=""><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><div style="word-wrap: break-word;" class=""><br class=""><div class=""><blockquote type="cite" class=""><div class=""><div class="">On Aug 1, 2014, at 1:44 AM, Gábor Horváth <<a href="mailto:xazax.hun@gmail.com" target="_blank" class="">xazax.hun@gmail.com</a>> wrote:</div><br class=""></div><div class=""><div dir="ltr" class=""><div class="">Hi Ted,<br class=""><div class="gmail_extra"><br class=""></div><div class="gmail_extra">Thank you for the review.<br class=""></div></div><div class="gmail_extra"><br class=""><div class=""><div class="gmail_quote">On 1 August 2014 07:25, Ted Kremenek<span class="Apple-converted-space"> </span><span dir="ltr" class=""><<a href="mailto:kremenek@apple.com" target="_blank" class="">kremenek@apple.com</a>></span><span class="Apple-converted-space"> </span>wrote:<br class=""><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div style="word-wrap: break-word;" class=""><div class="">Hi Gábor,</div><div class=""><br class=""></div><div class="">This is looking good to me. Some minor nits/comments:</div><div class=""><br class=""></div><div class="">- Please add doxygen comments for the CodeInjector class.</div></div></blockquote><div class=""> </div><div class="">Done.<br class=""></div><div class=""> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div style="word-wrap: break-word;" class=""><div class="">- For the BugReporter patch, are there tests for that functionality change? I saw tests in the other patch, but not that one. It's fine to separate the review of that change before the primary change goes in, but I was curious.</div></div></blockquote><div class=""><br class=""></div><div class="">Well, it may be a bit complicated. I deleted some code in BugReporter to not to discard bug reports that are in a model file, and the plist part of the test case only pass if that patch is applied (if the patch is not applied the nullpointer dereference warning that has the position in the modelfile will be discarded. In the long term it would be better to report these errors elsewhere but it is not supported yet by the bugreporter patch). I can move the plist check into a separate testcase and add that case to the BugReporter patch instead. The division by zero test should work without the BugReporter patch.<br class=""></div></div></div></div></div></div></blockquote><div class=""><br class=""></div><div class="">Ok, this make sense. Can you clarify what you mean by "better to report these errors elsewhere"?</div><div class=""><br class=""></div></div></div></blockquote><div class=""><br class=""></div><div class="">It might be confusing for the user, if the execution path of the bug report contains a location that is inside a model file so the report contains codes and files that are not present in the analysed project. So it might be more user friendly if the locations that are inside a model file would be excluded from the reported execution path, however if this is not an issue I am ok with reporting those locations as well.<br class=""><br class=""></div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><div style="word-wrap: break-word;" class=""><div class=""><div class=""><blockquote type="cite" class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div style="word-wrap: break-word;" class=""><div class="">- As for breaking code in the 'extra' repository, LLVM-internal API is not sacrosanct. If we break the 'extra' projects we just need to update them, but I'm not certain if that is possible in this case.</div></div></blockquote><div class=""><br class=""></div><div class="">As far as I can remember it would be a straightforward fix in the extra repository. Clang-tidy calls CreateAnalysisConsumer.<br class=""></div></div></div></div></blockquote><div class=""><br class=""></div></div>Sounds good. Let's get the right API and just fix up clang-tidy.</div><div class=""><div class=""><div class="h5"><br class=""><blockquote type="cite" class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""></div><div class=""> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div style="word-wrap: break-word;" class=""><div class="">- For comments, please consistently use sentence casing and end with periods, and for type names use the appropriate casing. For example:</div><div class=""><br class=""></div><div class=""><div class=""><div class="">+ // modules create a separate compilerinstance for parsing modules, maybe it is</div><div class="">+ // for reason so I mimic this behavior</div><div class="">+ CompilerInstance Instance;</div><div class="">...</div><div class=""><br class=""></div></div></div><div class="">This comment looks a bit suspect, since it seems like a question to yourself. Here you use the word "I"; who is "I" in the context of this code? The comment also seems like an unanswered question. Is this a stale comment?</div><div class=""><br class=""></div></div></blockquote><div class=""><br class=""></div><div class="">Done, the comment was improved.<br class=""></div><div class=""> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div style="word-wrap: break-word;" class=""><div class="">Another example is this comment:</div><div class=""><br class=""></div><div class=""><div class="">+ // FIXME: double memoization is redundant. Here and in bodyfarm.</div><div class="">+ llvm::StringMap<Stmt *> Bodies;</div></div><div class=""><br class=""></div><div class="">This can be made slightly cleaner. For example:</div><div class=""><br class=""></div><div class=""><div class="">+ // FIXME: Double memorization is redundant, with</div><div class="">+ /// memoization both here and in BodyFarm.</div><div class="">+ llvm::StringMap<Stmt *> Bodies;</div></div></div></blockquote><div class=""><br class=""></div><div class="">Done. <span class="Apple-converted-space"> </span><br class=""><br class=""></div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div style="word-wrap: break-word;" class=""><div class="">- Only use doxygen comments for documentation. For example:</div><div class=""><br class=""></div><div class=""><div class="">+ if (notzero_notmodeled(p)) {</div><div class="">+ /// There is no information about the value of p, because</div><div class="">+ /// notzero_notmodeled is not modeled and the function definition</div><div class="">+ /// is not available.</div><div class="">+ int j = 5 / p; // expected-warning {{Division by zero}}</div><div class="">+ }</div></div><div class=""><br class=""></div><div class="">In this case we should use '//', not '///'. The former are true comments, and the latter are candidates to be extracted for documentation.</div><div class=""><br class=""></div></div></blockquote><div class=""><br class=""></div><div class="">Done.<br class=""></div><div class=""> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div style="word-wrap: break-word;" class=""><div class="">Overall, however, this is getting really close.</div><div class=""><br class=""></div></div></blockquote><div class=""><br class=""></div><div class="">It is great.<br class=""><br class="">Thanks,<br class=""></div><div class="">Gábor<br class=""></div></div></div></div></blockquote><div class=""><br class=""></div></div></div><div class="">Wonderful. The rest of my comments are minor:</div><div class=""><br class=""></div><div class=""><div class=""><blockquote type="cite" class=""><div class="">+/// \brief CodeInjector is an interface which is responsible forinjecting AST of</div><div class="">+/// function definitions that may not be available in the original source.</div><div class="">+///</div><div class="">+/// The getBody function will be called each time the static analyzer examines a</div><div class="">+/// function call that has no definition available in the current translation</div><div class="">+/// unit. If the returned statement is not a nullpointer, it is assumed to be</div><div class="">+/// the body of a function which will be used for the analysis. The source of</div><div class="">+/// the body can be arbitrary, but it is advised to use memoization to avoid</div><div class="">+/// unnecessary reparsing of the external source that provides the body of the</div><div class="">+/// functions.</div></blockquote></div></div><div class=""><br class=""></div><div class=""> <span class="Apple-converted-space"> </span>"forinjecting" -> "for injecting"</div><div class=""> <span class="Apple-converted-space"> </span>"nullpointer" -> "null pointer"</div><div class=""><br class=""></div><div class=""><div class=""><blockquote type="cite" class=""><div class="">+++ include/clang/StaticAnalyzer/Frontend/FrontendActions.h (working copy)</div><div class="">@@ -10,10 +10,16 @@</div><div class=""> #ifndef LLVM_CLANG_GR_FRONTENDACTIONS_H</div><div class=""> #define LLVM_CLANG_GR_FRONTENDACTIONS_H</div><div class=""> </div><div class="">+#include <map></div><div class="">+</div></blockquote></div><div class=""><br class=""></div><div class="">This "#include" of <map> doesn't seem needed. Neither is the one in ModelConsumer.h</div><div class=""><br class=""></div><div class=""><div class=""><blockquote type="cite" class=""><div class=""><div class="">+++ lib/StaticAnalyzer/Frontend/ModelConsumer.cpp (working copy)</div><div class="">@@ -0,0 +1,42 @@</div><div class="">+//===--- ModelConsumer.cpp - ASTConsumer for consuming model files --------===//</div><div class="">+//</div><div class="">+// The LLVM Compiler Infrastructure</div><div class="">+//</div><div class="">+// This file is distributed under the University of Illinois Open Source</div><div class="">+// License. See LICENSE.TXT for details.</div><div class="">+//</div><div class="">+//===----------------------------------------------------------------------===//</div><div class="">+///</div><div class="">+/// \file</div><div class="">+/// \brief This file implements an ASTConsumer for consuming model files.</div><div class="">+///</div><div class="">+/// This ASTConsumer handles the AST of a parsed model file. All top level</div><div class="">+/// function definitions will be collected from that model file for later</div><div class="">+/// retrieval during the static analyzis. The body of these functions will not</div><div class="">+/// be injected into the ASTUnit of the analyzed translation unit. It will be</div><div class="">+/// available through the BodyFarm which is utilized by the AnalysisDeclContext</div><div class="">+/// class.</div><div class="">+///</div></div></blockquote><div class=""><div class=""><div class=""><br class=""></div><div class=""> <span class="Apple-converted-space"> </span>"analyzis" -> "analysis"</div><div class=""><br class=""></div><div class=""><blockquote type="cite" class=""><div class="">+ // The instance wants to take ownership, however disablefree frontend option</div><div class="">+ // is set to true to avoid double free issues</div></blockquote><br class=""></div><div class="">Use the actual casing for the option for technical precision:</div><div class=""><br class=""></div><div class=""> <span class="Apple-converted-space"> </span>DisableFree</div><div class=""><br class=""></div><div class=""><blockquote type="cite" class=""><div class="">+ /// \brief Synthetize a body for a declaration</div><div class="">+ ///</div><div class="">+ /// This method first looks up the appropriate model file based on the</div><div class="">+ /// model-path configuration option and the name of the declaration that is</div><div class="">+ /// looked up. If no model were synthetized yet for a function with that name</div><div class="">+ /// it will create a new compiler instance to parse the model file using the</div><div class="">+ /// ASTContext, Preprocessor, SourceManager of the original compiler instance.</div><div class="">+ /// The former resources are shared between the two compiler instance, so the</div><div class="">+ /// newly created instance have to "leak" these objects, since they are owned</div><div class="">+ /// by the original instance.</div></blockquote><br class=""></div><div class=""> Synthetize -> Synthesize</div><div class=""> synthetized -> synthesized</div><div class=""><br class=""></div><div class=""><blockquote type="cite" class=""><div class="">+ std::vector<std::unique_ptr<ASTUnit> > ModelAsts;</div></blockquote><div class=""><div class=""><br class=""></div></div><div class="">I'd prefer this to be "ModelASTs", as 'AST' is an acronym.</div><div class=""><br class=""></div></div></div></div></div></div></div></div></div></blockquote><div class=""><br class=""></div><div class="">It was an unused member (from an earlier implementation) that I forgot to remove, but done now.<br class=""></div><div class=""> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><div style="word-wrap: break-word;" class=""><div class=""><div class=""><div class=""><div class=""><div class=""><div class=""><div class=""><div class=""></div><div class="">Otherwise, this all looks great to me.</div><div class=""><br class=""></div></div></div></div></div></div></div><blockquote type="cite" class=""><div class=""><div class="h5"><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div style="word-wrap: break-word;" class=""><div class="">Cheers,</div><div class="">Ted</div><br class=""><div class=""><div class=""><div class=""><div class="">On Jul 30, 2014, at 3:29 AM, Gábor Horváth <<a href="mailto:xazax.hun@gmail.com" target="_blank" class="">xazax.hun@gmail.com</a>> wrote:</div><br class=""></div></div><blockquote type="cite" class=""><div class=""><div class=""><div dir="ltr" class=""><div class="">Hi Ted,<br class=""><br class=""></div>Thank you for the review.<div class="gmail_extra"><br class=""><br class=""><div class="gmail_quote">On 30 July 2014 08:18, Ted Kremenek<span class="Apple-converted-space"> </span><span dir="ltr" class=""><<a href="mailto:kremenek@apple.com" target="_blank" class="">kremenek@apple.com</a>></span><span class="Apple-converted-space"> </span>wrote:<br class=""><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div style="word-wrap: break-word;" class=""><div class="">Hi Gábor,</div><div class=""><br class=""></div><div class="">Thanks for making progress on this very promising enhancement to the analyzer. I have an assortment of comments, in no particular order:</div><div class=""><br class=""></div><div class="">- ModelInjector.h and ModelConsumer.h</div><div class=""><br class=""></div><div class="">There is a comment at the top of these files, but I think a bit more explanation is needed. For example:</div><div class=""><br class=""></div><div class=""> <span class="Apple-converted-space"> </span>MetaConsumer.cpp:</div><div class=""><br class=""></div><div class=""> <span class="Apple-converted-space"> </span>+// "Meta" ASTConsumer for consuming model files.</div><div class=""><br class=""></div><div class="">That doesn't really explain anything. What does "Meta" in quotes mean? I think an explanation here on what this does is helpful when someone discovers this code for the first time.</div><div class=""><br class=""></div><div class="">Similarly, we should add some high-level comments for CodeInjector.h and ModelInjector.h. We have a good start in ModelInjector.h:</div><div class=""><br class=""></div><div class=""><div class="">+/// \file</div><div class="">+/// \brief Defines the clang::ento::ModelInjector class which implements the</div><div class="">+/// clang::CodeInjector interface. This class is responsible for injecting</div><div class="">+/// function definitions that were synthetized from model files.</div><div class="">+///</div></div><div class=""><br class=""></div><div class="">Let's consider expanding it:</div><div class=""><br class=""></div><div class=""><div class=""> /// \brief This file defines the clang::ento::ModelInjector class which implements the</div><div class=""> /// clang::CodeInjector interface. This class is responsible for injecting</div><div class=""> /// function definitions that were synthesized from model files.</div></div><div class=""><br class=""></div><div class=""><div class=""> /// Model files allow definitions of functions to be lazily constituted for functions</div><div class=""> /// which lack bodies in the original source code. This allows the analyzer</div></div><div class=""> /// to more precisely analyze code that calls such functions, analyzing the</div><div class=""> /// artificial definitions (which typically approximate the semantics of the</div><div class=""> /// called function) when called by client code. These definitions are</div><div class=""> /// reconstituted lazily, on-demand, by the static analyzer engine.</div><div class=""><br class=""></div><div class="">CodeInjector.h provides some information, but it is a bit vague:</div><div class=""><br class=""></div><div class=""><div class="">+///</div><div class="">+/// \file</div><div class="">+/// \brief Defines the clang::CodeInjector interface which is responsible for</div><div class="">+/// injecting AST of function definitions from external source.</div><div class="">+///</div></div><div class=""><br class=""></div><div class="">It's a bit unclear how this gets used. I think a bit of prose here would help clarify its role in the static analyzer. I also think the CodeInjector interface is also more abstract than the prose describes. There's nothing about CodeInjector's interface that requires the injected definitions to come from an external source. That's an implementation detail of a concrete subclass. Instead, all CodeInjector does is provide an interface that lazily provides definitions for functions and methods that may not be present in the original source.</div></div></blockquote><div class=""><br class=""></div><div class="">I have added some further documentation to address these issues.<br class=""></div><div class=""><br class=""> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div style="word-wrap: break-word;" class=""><div class=""><br class=""></div><div class="">I'm also looking at the change to StaticAnalyzer/Frontend/FrontendActions.cpp, and wonder if we can simplify things:</div><div class=""><br class=""></div><div class=""><blockquote type="cite" class=""><div class="">+++ lib/StaticAnalyzer/Frontend/FrontendActions.cpp (working copy)</div><div class="">@@ -7,9 +7,11 @@</div><div class=""> //</div><div class=""> //===----------------------------------------------------------------------===//</div><div class=""> </div><div class="">+#include "clang/Frontend/CompilerInstance.h"</div><div class=""> #include "clang/StaticAnalyzer/Frontend/FrontendActions.h"</div><div class="">-#include "clang/Frontend/CompilerInstance.h"</div><div class=""> #include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"</div><div class="">+#include "clang/StaticAnalyzer/Frontend/ModelConsumer.h"</div><div class="">+#include "ModelInjector.h"</div><div class=""> using namespace clang;</div><div class=""> using namespace ento;</div><div class=""> </div><div class="">@@ -18,6 +20,14 @@</div><div class=""> return CreateAnalysisConsumer(CI.getPreprocessor(),</div><div class=""> CI.getFrontendOpts().OutputFile,</div><div class=""> CI.getAnalyzerOpts(),</div><div class="">- CI.getFrontendOpts().Plugins);</div><div class="">+ CI.getFrontendOpts().Plugins,</div><div class="">+ new ModelInjector(CI));</div><div class=""> }</div><div class=""> </div></blockquote></div><div class=""><br class=""></div><div class="">It looks like CreateAnalysisConsumer just continues to grow more arguments, all which derive from using 'CI'. This seems silly, since this function is called in one place. Instead of intro ducting a dependency on ModelInjector.h in this file, we can just sink these arguments into CreateAnalysisConsumer() itself, resulting in:</div><div class=""><br class=""></div><div class=""> <span class="Apple-converted-space"> </span>return CreateAnalysisConsumer(CI);</div><div class=""><br class=""></div><div class="">and let CreateAnalysisConsumer() do all that boilerplate.</div></div></blockquote><div class=""><br class=""></div><div class="">That was my original idea as well but it broke the compilation of some code in extra repository and I wasn't sure if it is ok to break the API with this patch. But I find it cleaner this way so I modified it in this iteration.<br class=""><br class=""></div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div style="word-wrap: break-word;" class=""><div class=""><br class=""></div><div class="">Next, let's look at the change to FrontendAction:</div><div class=""><br class=""></div><div class=""><blockquote type="cite" class=""><div class=""> class FrontendAction {</div><div class="">+ /// Is this action invoked on a model file? Model files are incomplete</div><div class="">+ /// translation units that relies on type information from another translation</div><div class="">+ /// unit. Check ParseModelFileAction for details.</div><div class="">+ bool ModelFile;</div></blockquote><div class=""><br class=""></div><div class="">Perhaps "IsModelFile"? "ModelFile" sounds like it should be a reference to the file itself.</div><br class=""><blockquote type="cite" class=""><div class=""> FrontendInputFile CurrentInput;</div><div class=""> std::unique_ptr<ASTUnit> CurrentASTUnit;</div><div class=""> CompilerInstance *Instance;</div><div class="">@@ -105,7 +109,11 @@</div><div class=""> /// @}</div><div class=""> </div><div class=""> public:</div><div class="">- FrontendAction();</div><div class="">+ /// \brief Constructor</div><div class="">+ ///</div><div class="">+ /// \param modelFile determines whether the source files this action invoked</div><div class="">+ /// on should be treated as a model file. Defaults to false.</div><div class="">+ FrontendAction(bool modelFile = false);</div></blockquote><div class=""><br class=""></div><div class="">It seems suboptimal to modify the interface of FrontendAction just for this one edge case. Instead of modifying the constructor arguments, we could default initialize "IsModelFile" to false, and have a setter to change it. For example:</div></div><div class=""><br class=""></div><div class=""> <span class="Apple-converted-space"> </span>ParseModelFileAction::ParseModelFileAction(llvm::StringMap<Stmt *> &Bodies)</div><div class=""> <span class="Apple-converted-space"> </span>: ASTFrontendAction(/*ModelFile=*/true), Bodies(Bodies) {}</div><div class=""><br class=""></div><div class="">becomes:</div><div class=""> </div><div class=""> <span class="Apple-converted-space"> </span>ParseModelFileAction::ParseModelFileAction(llvm::StringMap<Stmt *> &Bodies)</div><div class=""> <span class="Apple-converted-space"> </span>: Bodies(Bodies) {</div><div class=""> <span class="Apple-converted-space"> </span>IsModelFile = true;</div><div class=""> <span class="Apple-converted-space"> </span>}</div><div class=""><br class=""></div><div class="">Looking at this more, I wonder if we should modify FrontendAction at all. The only place where isModelParsingAction() is called is in one spot in CompilerInstance.cpp:</div><div class=""><br class=""></div><div class=""> if (hasSourceManager() && !Act.isModelParsingAction())</div><div class=""><br class=""></div><div class="">It *might* be cleaner to just have a virtual member function in FrontendAction, which defaults to returning false, but is generic for all subclasses to override. Then we don't need the "IsModelFile" field in FrontendAction at all, and we just have ParseModelFileAction override that single member function. We could then name that method to be something a bit more generic. That would allow us to not touch FrontendAction at all except for providing that single virtual method that can be overridden in subclasses. I somewhat prefer this approach because it provides a cleaner separation of concerns between FrontendAction (which is defined libFrontend) and the static analyzer. That would also allow you to get rid of isModelParsingAction() entirely (replacing it with something more generic).</div><div class=""><br class=""></div></div></blockquote><div class=""><br class=""></div><div class="">You are right, it is much cleaner to use a virtual function, so I modified the patch to use that approach. The new virtual function has the same name because I have yet to find any better and more general name yet. Do you have an idea for a better name?<br class=""></div><div class=""> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div style="word-wrap: break-word;" class=""><div class="">As for the test case:</div><div class=""><br class=""></div><div class=""><blockquote type="cite" class=""><div class="">+typedef int* intptr;</div><div class="">+</div><div class="">+void modelled(intptr p);</div><div class="">+</div><div class="">+int main() {</div><div class="">+ modelled(0);</div><div class="">+ return 0;</div><div class="">+}</div></blockquote><br class=""></div><div class="">Please add some comments in this test file explaining what is happening. Also, it would be great if this both used FileCheck (which it does now) but also verified the diagnostics so we get cross-checking of the output (we see this in some analyzer tests). It also makes it easier to understand the test.</div><div class=""><br class=""></div><div class="">Also, is there a reason to break up the tests between model-suppress-falsepos.cpp and model-file.cpp? It seems like one test file will do fine; just clearly comment on what is happening for each test. I also recommend called the modeled function "modeledFunction" instead of "modelled" (which according to my spell checker has an additional 'l'). </div></div></blockquote><div class=""><br class=""></div><div class="">I have merged the test files and also added some commets to explain what is going on. I have fixed the misspelling as well. The nullpointer dereference is only checked through plist because the point where the comment with the expected warning should be added is inside the model file and it did not work for me if the comment was in a separate file. If there is a different way to verify the warnings that are in a separate file and I did not find it, please let me know.<br class=""> <br class=""></div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div style="word-wrap: break-word;" class=""><div class=""><br class=""></div><div class="">As for the model files themselves:</div><div class=""><br class=""></div><div class=""><blockquote type="cite" class=""><div class="">Index: test/Analysis/modelled.model</div><div class="">===================================================================</div><div class="">--- test/Analysis/modelled.model (revision 0)</div><div class="">+++ test/Analysis/modelled.model (working copy)</div><div class="">@@ -0,0 +1,3 @@</div><div class="">+void modelled(intptr p) {</div><div class="">+ ++*p;</div><div class="">+}</div><div class="">\ No newline at end of file</div><div class="">Index: test/Analysis/notzero.model</div><div class="">===================================================================</div><div class="">--- test/Analysis/notzero.model (revision 0)</div><div class="">+++ test/Analysis/notzero.model (working copy)</div></blockquote><br class=""></div><div class="">Let's put these in a separate subdirectory, for example, "models", instead of mixing them with the tests. This way they really serve as "inputs" to the analyzer.</div></div></blockquote><div class=""><br class=""></div><div class="">I have moved the model files to tests/Inputs/Models.<br class=""></div><div class=""> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div style="word-wrap: break-word;" class=""><div class=""><br class=""></div><div class="">Overall this is looking good. I think the explanatory comments will really help people understand what this is doing, and I think changing how we thread the information through FrontendAction will help not introduce an artificial tainting of FrontendAction with concepts specific to the static analyzer.</div><div class=""><br class=""></div><div class="">Cheers,</div><div class="">Ted</div><div class=""><br class=""></div></div></blockquote><div class=""><br class=""></div><div class="">Thanks,<br class=""></div><div class="">Gábor<br class=""></div><div class=""> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div style="word-wrap: break-word;" class=""><br class=""><div class=""><div class=""><div class=""><div class="">On Jul 16, 2014, at 2:45 AM, Gábor Horváth <<a href="mailto:xazax.hun@gmail.com" target="_blank" class="">xazax.hun@gmail.com</a>> wrote:</div><br class=""></div></div><blockquote type="cite" class=""><div class=""><div class=""><div dir="ltr" class=""><br class=""><div class="gmail_extra"><br class=""><br class=""><div class="gmail_quote">On 14 July 2014 19:32, Anna Zaks<span class="Apple-converted-space"> </span><span dir="ltr" class=""><<a href="mailto:ganna@apple.com" target="_blank" class="">ganna@apple.com</a>></span><span class="Apple-converted-space"> </span>wrote:<br class=""><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div style="word-wrap: break-word;" class=""><br class=""><div class=""><div class=""><blockquote type="cite" class=""><div class="">On Jul 13, 2014, at 6:11 AM, Gábor Horváth <<a href="mailto:xazax.hun@gmail.com" target="_blank" class="">xazax.hun@gmail.com</a>> wrote:</div><br class=""><div class=""><div dir="ltr" class=""><div class="">Hi Anna,<br class=""><br class=""></div>Thank you for the review. I have tweaked the test, so it no longer requires the error reporting tweak that is not done yet to pass. I have also added some high level comments to some files, if you think some information is lacking I will add them in the next iteration as well. The BugReporter patch is now separated into a different patch.<span class="Apple-converted-space"> </span><br class=""><div class="gmail_extra"><br class=""><br class=""><div class="gmail_quote">On 11 July 2014 18:02, Anna Zaks<span class="Apple-converted-space"> </span><span dir="ltr" class=""><<a href="mailto:ganna@apple.com" target="_blank" class="">ganna@apple.com</a>></span><span class="Apple-converted-space"> </span>wrote:<br class=""><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div style="word-wrap: break-word;" class=""><div class=""><br class=""></div><div class="">For example, modeling functions should allow you to find bugs and suppress false positives outside of those functions. I would suggest adding a few of those tests first.<br class=""><div class=""><br class=""></div></div></div></blockquote><div class=""><br class=""></div></div></div></div></div></blockquote><blockquote type="cite" class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class="">How are the false positives suppressed? I did not find any resource on that. Found some analyzer attributes but I did not find them suitable for this purpuse at the first glance. But I think once the locations that are in a model file are omitted from the report path, the regular methods for suppressing false positives should work (and I will definitely add test case to ensure this once it is done).<br class=""></div><div class=""><br class=""></div></div></div></div></blockquote><div class=""><br class=""></div></div>What I meant is that it is possible to construct a test where ability to model a function would eliminate a false positive. This would be another way to test your patch without worrying about BugReporter.<br class=""></div></div></blockquote><div class=""><br class=""></div><div class="">I got it now, thansk. I have updated the patch with a test case where a false positive case is eliminated by a model file.<br class=""><br class=""></div><div class="">Thanks,<br class=""></div><div class="">Gábor<br class=""></div><div class=""> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div style="word-wrap: break-word;" class=""><div class=""><blockquote type="cite" class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class="">Thanks,<br class=""></div><div class="">Gábor<br class=""></div></div></div></div><span class=""><api_modeling.patch></span><span class=""><bugreporter.patch></span></blockquote></div><br class=""></div></blockquote></div><br class=""></div></div></div></div><span class=""><api_modeling.patch></span><span class=""><bugreporter.patch></span></blockquote></div><br class=""></div></blockquote></div><br class=""></div></div></div></div><span class=""><api_modeling.patch></span><span class=""><bugreporter.patch></span></blockquote></div><br class=""></div></blockquote></div><br class=""></div></div></div></div><span class=""><api_modeling.patch></span><span class=""><bugreporter.patch></span></blockquote></div><br class=""></div></blockquote></div><br class=""><br class=""></div><div class="gmail_extra">Thanks,<br class="">Gábor<br class=""></div></div><span id="cid:6FB1B4CD-F670-4239-9F7F-984583234147@hsd1.ca.comcast.net."><api_modeling.patch></span><span id="cid:33E6FABC-AAB0-4885-BFFD-57E0511102E5@hsd1.ca.comcast.net."><bugreporter.patch></span><span id="cid:74C654CA-8890-4D5B-A495-45E6B1D6E230@hsd1.ca.comcast.net."><clangTidy.patch></span></div></blockquote></div><br class=""></body></html>