<div dir="ltr">Hi Ted,<br><br>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><div class="gmail_extra"><br><br><div class="gmail_quote">
On 12 August 2014 08:44, Ted Kremenek <span dir="ltr"><<a href="mailto:kremenek@apple.com" target="_blank">kremenek@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><br><div><blockquote type="cite"><div class=""><div>On Aug 1, 2014, at 1:44 AM, Gábor Horváth <<a href="mailto:xazax.hun@gmail.com" target="_blank">xazax.hun@gmail.com</a>> wrote:</div>
<br></div><div><div dir="ltr"><div class="">Hi Ted,<br><div class="gmail_extra"><br></div><div class="gmail_extra">Thank you for the review.<br></div></div><div class="gmail_extra"><br><div class=""><div class="gmail_quote">
On 1 August 2014 07:25, Ted Kremenek <span dir="ltr"><<a href="mailto:kremenek@apple.com" target="_blank">kremenek@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"><div>Hi Gábor,</div><div><br></div><div>This is looking good to me.  Some minor nits/comments:</div>

<div><br></div><div>- Please add doxygen comments for the CodeInjector class.</div></div></blockquote><div> </div><div>Done.<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>-
 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><br></div><div>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></div></div></div></div></div></div></blockquote><div><br></div><div>Ok, this make sense.  Can you clarify what you mean by "better to report these errors elsewhere"?</div><div class="">
<br></div></div></div></blockquote><div><br></div><div>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>
 <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div class=""><blockquote type="cite"><div dir="ltr"><div class="gmail_extra">
<div class="gmail_quote"><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>-
 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><br></div><div>As far as I can remember it would be a straightforward fix in the extra repository. Clang-tidy calls CreateAnalysisConsumer.<br>
</div></div></div></div></blockquote><div><br></div></div>Sounds good.  Let's get the right API and just fix up clang-tidy.</div><div><div><div class="h5"><br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra">
<div class="gmail_quote"><div>
</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>-
 For comments, please consistently use sentence casing and end with 
periods, and for type names use the appropriate casing.  For example:</div><div><br></div><div><div><div>+  // modules create a separate compilerinstance for parsing modules, maybe it is</div><div>+  // for reason so I mimic this behavior</div>

<div>+  CompilerInstance Instance;</div><div>...</div><div><br></div></div></div><div>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><br></div></div></blockquote><div><br></div><div>Done, the comment was improved.<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>Another example is this comment:</div><div><br></div><div><div>+  // FIXME: double memoization is redundant. Here and in bodyfarm.</div><div>+  llvm::StringMap<Stmt *> Bodies;</div>

</div><div><br></div><div>This can be made slightly cleaner.  For example:</div><div><br></div><div><div>+  // FIXME: Double memorization is redundant, with</div><div>+  /// memoization both here and in BodyFarm.</div><div>

+  llvm::StringMap<Stmt *> Bodies;</div></div></div></blockquote><div><br></div><div>Done.  <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>- Only use doxygen comments for documentation.  For example:</div><div><br></div><div><div>+  if (notzero_notmodeled(p)) {</div><div>+   /// There is no information about the value of p, because</div>

<div>+   /// notzero_notmodeled is not modeled and the function definition</div><div>+   /// is not available.</div><div>+    int j = 5 / p; // expected-warning {{Division by zero}}</div><div>+  }</div></div><div><br></div>

<div>In
 this case we should use '//', not '///'.  The former are true comments,
 and the latter are candidates to be extracted for documentation.</div><div><br></div></div></blockquote><div><br></div><div>Done.<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>Overall, however, this is getting really close.</div><div><br></div></div></blockquote><div><br></div><div>It is great.<br><br>Thanks,<br></div><div>Gábor<br></div></div></div></div>
</blockquote><div><br></div></div></div><div>Wonderful.  The rest of my comments are minor:</div><div><br></div><div><div><blockquote type="cite"><div>+/// \brief CodeInjector is an interface which is responsible forinjecting AST of</div>
<div>+/// function definitions that may not be available in the original source.</div><div>+///</div><div>+/// The getBody function will be called each time the static analyzer examines a</div><div>+/// function call that has no definition available in the current translation</div>
<div>+/// unit. If the returned statement is not a nullpointer, it is assumed to be</div><div>+/// the body of a function which will be used for the analysis. The source of</div><div>+/// the body can be arbitrary, but it is advised to use memoization to avoid</div>
<div>+/// unnecessary reparsing of the external source that provides the body of the</div><div>+/// functions.</div></blockquote></div></div><div><br></div><div>  "forinjecting" -> "for injecting"</div>
<div>  "nullpointer" -> "null pointer"</div><div><br></div><div><div><blockquote type="cite"><div>+++ include/clang/StaticAnalyzer/Frontend/FrontendActions.h (working copy)</div><div>@@ -10,10 +10,16 @@</div>
<div> #ifndef LLVM_CLANG_GR_FRONTENDACTIONS_H</div><div> #define LLVM_CLANG_GR_FRONTENDACTIONS_H</div><div> </div><div>+#include <map></div><div>+</div></blockquote></div><div><br></div><div>This "#include" of <map> doesn't seem needed.  Neither is the one in ModelConsumer.h</div>
<div><br></div><div><div><blockquote type="cite"><div><div>+++ lib/StaticAnalyzer/Frontend/ModelConsumer.cpp (working copy)</div><div>@@ -0,0 +1,42 @@</div><div>+//===--- ModelConsumer.cpp - ASTConsumer for consuming model files --------===//</div>
<div>+//</div><div>+//                     The LLVM Compiler Infrastructure</div><div>+//</div><div>+// This file is distributed under the University of Illinois Open Source</div><div>+// License. See LICENSE.TXT for details.</div>
<div>+//</div><div>+//===----------------------------------------------------------------------===//</div><div>+///</div><div>+/// \file</div><div>+/// \brief This file implements an ASTConsumer for consuming model files.</div>
<div>+///</div><div>+/// This ASTConsumer handles the AST of a parsed model file. All top level</div><div>+/// function definitions will be collected from that model file for later</div><div>+/// retrieval during the static analyzis. The body of these functions will not</div>
<div>+/// be injected into the ASTUnit of the analyzed translation unit. It will be</div><div>+/// available through the BodyFarm which is utilized by the AnalysisDeclContext</div><div>+/// class.</div><div>+///</div></div>
</blockquote><div><div><div><br></div><div>  "analyzis" -> "analysis"</div><div><br></div><div><blockquote type="cite"><div>+  // The instance wants to take ownership, however disablefree frontend option</div>
<div>+  // is set to true to avoid double free issues</div></blockquote><br></div><div>Use  the actual casing for the option for technical precision:</div><div><br></div><div>  DisableFree</div><div><br></div><div><blockquote type="cite">
<div>+  /// \brief Synthetize a body for a declaration</div><div>+  ///</div><div>+  /// This method first looks up the appropriate model file based on the</div><div>+  /// model-path configuration option and the name of the declaration that is</div>
<div>+  /// looked up. If no model were synthetized yet for a function with that name</div><div>+  /// it will create a new compiler instance to parse the model file using the</div><div>+  /// ASTContext, Preprocessor, SourceManager of the original compiler instance.</div>
<div>+  /// The former resources are shared between the two compiler instance, so the</div><div>+  /// newly created instance have to "leak" these objects, since they are owned</div><div>+  /// by the original instance.</div>
</blockquote><br></div><div>   Synthetize -> Synthesize</div><div>  synthetized -> synthesized</div><div><br></div><div><blockquote type="cite"><div>+  std::vector<std::unique_ptr<ASTUnit> > ModelAsts;</div>
</blockquote><div><div><br></div></div><div>I'd prefer this to be "ModelASTs", as 'AST' is an acronym.</div><div><br></div></div></div></div></div></div></div></div></div></blockquote><div><br></div>
<div>It was an unused member (from an earlier implementation) that I forgot to remove, but done now.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><div><div><div><div><div><div><div></div><div>Otherwise, this all looks great to me.</div><div><br></div></div></div></div></div></div></div><blockquote type="cite"><div><div class="h5">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><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>Cheers,</div><div>Ted</div><br><div><div><div><div>On Jul 30, 2014, at 3:29 AM, Gábor Horváth <<a href="mailto:xazax.hun@gmail.com" target="_blank">xazax.hun@gmail.com</a>> wrote:</div>

<br></div></div><blockquote type="cite"><div><div><div dir="ltr"><div>Hi Ted,<br><br></div>Thank you for the review.<div class="gmail_extra"><br><br><div class="gmail_quote">On 30 July 2014 08:18, Ted Kremenek <span dir="ltr"><<a href="mailto:kremenek@apple.com" target="_blank">kremenek@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"><div>Hi Gábor,</div><div><br></div><div>Thanks
 for making progress on this very promising enhancement to the analyzer.
  I have an assortment of comments, in no particular order:</div>
<div><br></div><div>- ModelInjector.h and ModelConsumer.h</div><div><br></div><div>There is a comment at the top of these files, but I think a bit more explanation is needed.  For example:</div><div><br></div><div>  MetaConsumer.cpp:</div>


<div><br></div><div>    +// "Meta" ASTConsumer for consuming model files.</div><div><br></div><div>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><br></div><div>Similarly, we should add some high-level comments 
for CodeInjector.h and ModelInjector.h.  We have a good start in 
ModelInjector.h:</div><div><br></div><div><div>+/// \file</div><div>+/// \brief Defines the clang::ento::ModelInjector class which implements the</div>
<div>+/// clang::CodeInjector interface. This class is responsible for injecting</div><div>+/// function definitions that were synthetized from model files.</div><div>+///</div></div><div><br></div><div>Let's consider expanding it:</div>


<div><br></div><div><div> /// \brief This file defines the clang::ento::ModelInjector class which implements the</div><div> /// clang::CodeInjector interface. This class is responsible for injecting</div><div> /// function definitions that were synthesized from model files.</div>


</div><div><br></div><div><div> /// Model files allow definitions of functions to be lazily constituted for functions</div><div> /// which lack bodies in the original source code.  This allows the analyzer</div></div><div>


 /// to more precisely analyze code that calls such functions, analyzing the</div><div> /// artificial definitions (which typically approximate the semantics of the</div><div> /// called function) when called by client code.  These definitions are</div>


<div> /// reconstituted lazily, on-demand, by the static analyzer engine.</div><div><br></div><div>CodeInjector.h provides some information, but it is a bit vague:</div><div><br></div><div><div>+///</div><div>+/// \file</div>


<div>+/// \brief Defines the clang::CodeInjector interface which is responsible for</div><div>+/// injecting AST of function definitions from external source.</div><div>+///</div></div><div><br></div><div>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><br></div><div>I have added some further documentation to address these issues.<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>I'm also looking at the change to StaticAnalyzer/Frontend/FrontendActions.cpp, and wonder if we can simplify things:</div><div><br></div><div><blockquote type="cite">


<div>+++ lib/StaticAnalyzer/Frontend/FrontendActions.cpp (working copy)</div><div>@@ -7,9 +7,11 @@</div><div> //</div><div> //===----------------------------------------------------------------------===//</div><div> </div>


<div>+#include "clang/Frontend/CompilerInstance.h"</div><div> #include "clang/StaticAnalyzer/Frontend/FrontendActions.h"</div><div>-#include "clang/Frontend/CompilerInstance.h"</div><div> #include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"</div>


<div>+#include "clang/StaticAnalyzer/Frontend/ModelConsumer.h"</div><div>+#include "ModelInjector.h"</div><div> using namespace clang;</div><div> using namespace ento;</div><div> </div><div>@@ -18,6 +20,14 @@</div>


<div>   return CreateAnalysisConsumer(CI.getPreprocessor(),</div><div>                                 CI.getFrontendOpts().OutputFile,</div><div>                                 CI.getAnalyzerOpts(),</div><div>-                                CI.getFrontendOpts().Plugins);</div>


<div>+                                CI.getFrontendOpts().Plugins,</div><div>+                                new ModelInjector(CI));</div><div> }</div><div> </div></blockquote></div><div><br></div><div>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><br></div><div>  return CreateAnalysisConsumer(CI);</div><div><br></div><div>and let CreateAnalysisConsumer() do all that boilerplate.</div></div></blockquote><div><br></div><div>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>
<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>Next, let's look at the change to FrontendAction:</div>


<div><br></div><div><blockquote type="cite"><div> class FrontendAction {</div><div>+  /// Is this action invoked on a model file? Model files are incomplete</div><div>+  /// translation units that relies on type information from another translation</div>


<div>+  /// unit. Check ParseModelFileAction for details.</div><div>+  bool ModelFile;</div></blockquote><div><br></div><div>Perhaps "IsModelFile"?  "ModelFile" sounds like it should be a reference to the file itself.</div>


<br><blockquote type="cite"><div>   FrontendInputFile CurrentInput;</div><div>   std::unique_ptr<ASTUnit> CurrentASTUnit;</div><div>   CompilerInstance *Instance;</div><div>@@ -105,7 +109,11 @@</div><div>   /// @}</div>


<div> </div><div> public:</div><div>-  FrontendAction();</div><div>+  /// \brief Constructor</div><div>+  ///</div><div>+  /// \param modelFile determines whether the source files this action invoked</div><div>+  /// on should be treated as a model file. Defaults to false.</div>


<div>+  FrontendAction(bool modelFile = false);</div></blockquote><div><br></div><div>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><br></div><div>  ParseModelFileAction::ParseModelFileAction(llvm::StringMap<Stmt *> &Bodies)</div><div>    : ASTFrontendAction(/*ModelFile=*/true), Bodies(Bodies) {}</div><div><br></div><div>becomes:</div>


<div> </div><div>  ParseModelFileAction::ParseModelFileAction(llvm::StringMap<Stmt *> &Bodies)</div><div>    : Bodies(Bodies)  {</div><div>    IsModelFile = true;</div><div>  }</div><div><br></div><div>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><br></div><div>   if (hasSourceManager() && !Act.isModelParsingAction())</div><div><br></div><div>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><br></div></div></blockquote><div><br></div><div>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>
</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>As for the test case:</div><div><br></div>

<div><blockquote type="cite">
<div>+typedef int* intptr;</div><div>+</div><div>+void modelled(intptr p);</div><div>+</div><div>+int main() {</div><div>+ modelled(0);</div><div>+ return 0;</div><div>+}</div></blockquote><br></div><div>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><br></div><div>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><br></div><div>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>
 <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>As for the model files themselves:</div>

<div><br></div>
<div><blockquote type="cite"><div>Index: test/Analysis/modelled.model</div><div>===================================================================</div><div>--- test/Analysis/modelled.model  (revision 0)</div><div>+++ test/Analysis/modelled.model  (working copy)</div>


<div>@@ -0,0 +1,3 @@</div><div>+void modelled(intptr p) {</div><div>+ ++*p;</div><div>+}</div><div>\ No newline at end of file</div><div>Index: test/Analysis/notzero.model</div><div>===================================================================</div>


<div>--- test/Analysis/notzero.model (revision 0)</div><div>+++ test/Analysis/notzero.model (working copy)</div></blockquote><br></div><div>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><br></div><div>I have moved the model files to tests/Inputs/Models.<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>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><br></div><div>Cheers,</div><div>Ted</div><div><br></div></div></blockquote><div><br></div><div>Thanks,<br></div><div>Gábor<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"><br><div><div><div><div>On Jul 16, 2014, at 2:45 AM, Gábor Horváth <<a href="mailto:xazax.hun@gmail.com" target="_blank">xazax.hun@gmail.com</a>> wrote:</div><br>
</div></div><blockquote type="cite"><div><div><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On 14 July 2014 19:32, 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><blockquote type="cite"><div>On Jul 13, 2014, at 6:11 AM, Gábor Horváth <<a href="mailto:xazax.hun@gmail.com" target="_blank">xazax.hun@gmail.com</a>> wrote:</div>

<br><div><div dir="ltr"><div>Hi Anna,<br><br></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. <br>


<div class="gmail_extra"><br><br><div class="gmail_quote">On 11 July 2014 18:02, 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"><div><br></div><div>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>


<div><br></div></div></div></blockquote><div><br></div></div></div></div></div></blockquote><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>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>


</div><div><br></div></div></div></div></blockquote><div><br></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>

</div></div></blockquote><div><br></div><div>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><br></div><div>Thanks,<br></div><div>Gábor<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><blockquote type="cite"><div dir="ltr"><div class="gmail_extra">


<div class="gmail_quote"><div>Thanks,<br></div><div>Gábor<br></div></div></div></div>
<span><api_modeling.patch></span><span><bugreporter.patch></span></blockquote></div><br></div></blockquote></div><br></div></div>
</div></div><span><api_modeling.patch></span><span><bugreporter.patch></span></blockquote></div><br></div></blockquote></div><br></div></div>
</div></div><span><api_modeling.patch></span><span><bugreporter.patch></span></blockquote></div><br></div></blockquote></div><br></div></div>
</div></div><span><api_modeling.patch></span><span><bugreporter.patch></span></blockquote></div><br></div></blockquote></div><br><br></div><div class="gmail_extra">Thanks,<br>Gábor<br></div></div>