Hi Ted, On 18 August 2014 08:28, Ted kremenek <[email protected]> wrote:
> Hi Gábor, > > These things happen, and it's a good lesson to learn at some point. :) > > It was a very effective way to learn that lesson for a lifetime :) > I myself build with RelWithDebInfo, but force assertions to be enabled. > I think I will do the same from now on. > From your explanation of the failures, it sounds like there is going to > be a bit more exploration to suss out the remaining changes, but I'm > confident that you'll get there. I don't have specific guidance on how to > solve the problems you mention; those may be worth bringing up on cfe-dev > if you have questions. > > My fixes for the problems I mentioned is already included in the patches attached to my former mail. > Cheers, > Ted > Thanks, Gábor > > On Aug 16, 2014, at 5:47 AM, Gábor Horváth <[email protected]> wrote: > > Hi Ted, > > It looks like I did a fatal mistake. To reduce compile and link times and > make the test suite faster to be able to iterate faster during the > development I used the RelWithDebInfo build type. Unfortunately it looks > like the asserts are turned off in this build type. However I did my best > to workaround the problems. > > On 14 August 2014 09:13, Ted Kremenek <[email protected]> wrote: > >> Hi Gábor, >> >> Great! I'm seeing crashes in diagnostic emission when running the tests: >> >> test: clang/test/Analysis/inlining/path-notes.m >> Assertion failed: (Loc.isValid()), function PathDiagnosticLocation, file >> clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h, line >> 186. >> >> > I did not notice my patch was the reason that made this patch fail. The > reason is, when a function body is synthesised from a hand built AST it has > no valid source locations and it asserts when such locations is reported. I > worked around this issue, so one now able to decide whether the code that > is synthesised is from a model file or hand written AST. > > >> I'm also seeing assertions when testing the model file: >> >> test: clang/test/Analysis/model-file.cpp >> Assertion failed: (MainFileID.isInvalid() && "MainFileID already set!"), >> function setMainFileID, file clang/include/clang/Basic/SourceManager.h, >> line 757. >> >> > The parsing of a file start with the preprocessor entering to the main > file. To be able to start parsing it is important to temporarily modify the > the main file ID, so I had no better idea than removing that assert. > However there were several other ones that were the result of reusing the > preprocessor of the main source. It was important to reuse that > preprocessor, because the identifier table from that preprocessor is > necessary to parse the model file and I did not found any good method to > merge those information into a new preprocessor instance. So I created two > new methods, one to prepare a preprocessor to parse a model file and one to > clean it up after the parsing of a such file. > > >> Are you not seeing these? >> >> > Sorry for my oversight about the turned off asserts, I guess we need to > iterate a bit more on the patch. > > >> Cheers, >> Ted >> >> > Thanks, > Gábor > > >> On Aug 13, 2014, at 11:35 PM, Gábor Horváth <[email protected]> wrote: >> >> Hi Ted, >> >> The ModelInjector is no longer instantiated when the modelpath is not set. >> >> Thanks, >> Gábor >> >> >> On 13 August 2014 07:11, Ted Kremenek <[email protected]> wrote: >> >>> Thanks Gábor. This looks great. >>> >>> 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. >>> >>> Specifically: >>> >>> std::unique_ptr<AnalysisASTConsumer> >>> -ento::CreateAnalysisConsumer(const Preprocessor &pp, const std::string >>> &outDir, >>> - AnalyzerOptionsRef opts, >>> - ArrayRef<std::string> plugins) { >>> +ento::CreateAnalysisConsumer(CompilerInstance &CI) { >>> // Disable the effects of '-Werror' when using the AnalysisConsumer. >>> - pp.getDiagnostics().setWarningsAsErrors(false); >>> + CI.getPreprocessor().getDiagnostics().setWarningsAsErrors(false); >>> >>> - return llvm::make_unique<AnalysisConsumer>(pp, outDir, opts, plugins); >>> + return llvm::make_unique<AnalysisConsumer>( >>> + CI.getPreprocessor(), CI.getFrontendOpts().OutputFile, >>> + CI.getAnalyzerOpts(), CI.getFrontendOpts().Plugins, >>> + new ModelInjector(CI)); >>> } >>> >>> >>> We can query 'opts' to see if model-path is empty; if it is we can pass >>> nullptr instead of 'new ModelInjector(CI)'. >>> >>> On Aug 12, 2014, at 1:22 AM, Gábor Horváth <[email protected]> wrote: >>> >>> Hi Ted, >>> >>> 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. >>> >>> >>> On 12 August 2014 08:44, Ted Kremenek <[email protected]> wrote: >>> >>>> >>>> On Aug 1, 2014, at 1:44 AM, Gábor Horváth <[email protected]> wrote: >>>> >>>> Hi Ted, >>>> >>>> Thank you for the review. >>>> >>>> On 1 August 2014 07:25, Ted Kremenek <[email protected]> wrote: >>>> >>>>> Hi Gábor, >>>>> >>>>> This is looking good to me. Some minor nits/comments: >>>>> >>>>> - Please add doxygen comments for the CodeInjector class. >>>>> >>>> >>>> Done. >>>> >>>> >>>>> - 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. >>>>> >>>> >>>> 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. >>>> >>>> >>>> Ok, this make sense. Can you clarify what you mean by "better to >>>> report these errors elsewhere"? >>>> >>>> >>> 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. >>> >>> >>>> >>>>> - 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. >>>>> >>>> >>>> As far as I can remember it would be a straightforward fix in the extra >>>> repository. Clang-tidy calls CreateAnalysisConsumer. >>>> >>>> >>>> Sounds good. Let's get the right API and just fix up clang-tidy. >>>> >>>> >>>> >>>>> - For comments, please consistently use sentence casing and end with >>>>> periods, and for type names use the appropriate casing. For example: >>>>> >>>>> + // modules create a separate compilerinstance for parsing modules, >>>>> maybe it is >>>>> + // for reason so I mimic this behavior >>>>> + CompilerInstance Instance; >>>>> ... >>>>> >>>>> 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? >>>>> >>>>> >>>> Done, the comment was improved. >>>> >>>> >>>>> Another example is this comment: >>>>> >>>>> + // FIXME: double memoization is redundant. Here and in bodyfarm. >>>>> + llvm::StringMap<Stmt *> Bodies; >>>>> >>>>> This can be made slightly cleaner. For example: >>>>> >>>>> + // FIXME: Double memorization is redundant, with >>>>> + /// memoization both here and in BodyFarm. >>>>> + llvm::StringMap<Stmt *> Bodies; >>>>> >>>> >>>> Done. >>>> >>>> - Only use doxygen comments for documentation. For example: >>>>> >>>>> + if (notzero_notmodeled(p)) { >>>>> + /// There is no information about the value of p, because >>>>> + /// notzero_notmodeled is not modeled and the function definition >>>>> + /// is not available. >>>>> + int j = 5 / p; // expected-warning {{Division by zero}} >>>>> + } >>>>> >>>>> In this case we should use '//', not '///'. The former are true >>>>> comments, and the latter are candidates to be extracted for documentation. >>>>> >>>>> >>>> Done. >>>> >>>> >>>>> Overall, however, this is getting really close. >>>>> >>>>> >>>> It is great. >>>> >>>> Thanks, >>>> Gábor >>>> >>>> >>>> Wonderful. The rest of my comments are minor: >>>> >>>> +/// \brief CodeInjector is an interface which is responsible >>>> forinjecting AST of >>>> +/// function definitions that may not be available in the original >>>> source. >>>> +/// >>>> +/// The getBody function will be called each time the static analyzer >>>> examines a >>>> +/// function call that has no definition available in the current >>>> translation >>>> +/// unit. If the returned statement is not a nullpointer, it is >>>> assumed to be >>>> +/// the body of a function which will be used for the analysis. The >>>> source of >>>> +/// the body can be arbitrary, but it is advised to use memoization to >>>> avoid >>>> +/// unnecessary reparsing of the external source that provides the >>>> body of the >>>> +/// functions. >>>> >>>> >>>> "forinjecting" -> "for injecting" >>>> "nullpointer" -> "null pointer" >>>> >>>> +++ include/clang/StaticAnalyzer/Frontend/FrontendActions.h (working >>>> copy) >>>> @@ -10,10 +10,16 @@ >>>> #ifndef LLVM_CLANG_GR_FRONTENDACTIONS_H >>>> #define LLVM_CLANG_GR_FRONTENDACTIONS_H >>>> >>>> +#include <map> >>>> + >>>> >>>> >>>> This "#include" of <map> doesn't seem needed. Neither is the one in >>>> ModelConsumer.h >>>> >>>> +++ lib/StaticAnalyzer/Frontend/ModelConsumer.cpp (working copy) >>>> @@ -0,0 +1,42 @@ >>>> +//===--- ModelConsumer.cpp - ASTConsumer for consuming model files >>>> --------===// >>>> +// >>>> +// The LLVM Compiler Infrastructure >>>> +// >>>> +// This file is distributed under the University of Illinois Open >>>> Source >>>> +// License. See LICENSE.TXT for details. >>>> +// >>>> >>>> +//===----------------------------------------------------------------------===// >>>> +/// >>>> +/// \file >>>> +/// \brief This file implements an ASTConsumer for consuming model >>>> files. >>>> +/// >>>> +/// This ASTConsumer handles the AST of a parsed model file. All top >>>> level >>>> +/// function definitions will be collected from that model file for >>>> later >>>> +/// retrieval during the static analyzis. The body of these functions >>>> will not >>>> +/// be injected into the ASTUnit of the analyzed translation unit. It >>>> will be >>>> +/// available through the BodyFarm which is utilized by the >>>> AnalysisDeclContext >>>> +/// class. >>>> +/// >>>> >>>> >>>> "analyzis" -> "analysis" >>>> >>>> + // The instance wants to take ownership, however disablefree >>>> frontend option >>>> + // is set to true to avoid double free issues >>>> >>>> >>>> Use the actual casing for the option for technical precision: >>>> >>>> DisableFree >>>> >>>> + /// \brief Synthetize a body for a declaration >>>> + /// >>>> + /// This method first looks up the appropriate model file based on >>>> the >>>> + /// model-path configuration option and the name of the declaration >>>> that is >>>> + /// looked up. If no model were synthetized yet for a function with >>>> that name >>>> + /// it will create a new compiler instance to parse the model file >>>> using the >>>> + /// ASTContext, Preprocessor, SourceManager of the original compiler >>>> instance. >>>> + /// The former resources are shared between the two compiler >>>> instance, so the >>>> + /// newly created instance have to "leak" these objects, since they >>>> are owned >>>> + /// by the original instance. >>>> >>>> >>>> Synthetize -> Synthesize >>>> synthetized -> synthesized >>>> >>>> + std::vector<std::unique_ptr<ASTUnit> > ModelAsts; >>>> >>>> >>>> I'd prefer this to be "ModelASTs", as 'AST' is an acronym. >>>> >>>> >>> It was an unused member (from an earlier implementation) that I forgot >>> to remove, but done now. >>> >>> >>>> Otherwise, this all looks great to me. >>>> >>>> >>>> >>>>> Cheers, >>>>> Ted >>>>> >>>>> On Jul 30, 2014, at 3:29 AM, Gábor Horváth <[email protected]> >>>>> wrote: >>>>> >>>>> Hi Ted, >>>>> >>>>> Thank you for the review. >>>>> >>>>> >>>>> On 30 July 2014 08:18, Ted Kremenek <[email protected]> wrote: >>>>> >>>>>> Hi Gábor, >>>>>> >>>>>> Thanks for making progress on this very promising enhancement to the >>>>>> analyzer. I have an assortment of comments, in no particular order: >>>>>> >>>>>> - ModelInjector.h and ModelConsumer.h >>>>>> >>>>>> There is a comment at the top of these files, but I think a bit more >>>>>> explanation is needed. For example: >>>>>> >>>>>> MetaConsumer.cpp: >>>>>> >>>>>> +// "Meta" ASTConsumer for consuming model files. >>>>>> >>>>>> 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. >>>>>> >>>>>> Similarly, we should add some high-level comments for CodeInjector.h >>>>>> and ModelInjector.h. We have a good start in ModelInjector.h: >>>>>> >>>>>> +/// \file >>>>>> +/// \brief Defines the clang::ento::ModelInjector class which >>>>>> implements the >>>>>> +/// clang::CodeInjector interface. This class is responsible for >>>>>> injecting >>>>>> +/// function definitions that were synthetized from model files. >>>>>> +/// >>>>>> >>>>>> Let's consider expanding it: >>>>>> >>>>>> /// \brief This file defines the clang::ento::ModelInjector class >>>>>> which implements the >>>>>> /// clang::CodeInjector interface. This class is responsible for >>>>>> injecting >>>>>> /// function definitions that were synthesized from model files. >>>>>> >>>>>> /// Model files allow definitions of functions to be lazily >>>>>> constituted for functions >>>>>> /// which lack bodies in the original source code. This allows the >>>>>> analyzer >>>>>> /// to more precisely analyze code that calls such functions, >>>>>> analyzing the >>>>>> /// artificial definitions (which typically approximate the >>>>>> semantics of the >>>>>> /// called function) when called by client code. These definitions >>>>>> are >>>>>> /// reconstituted lazily, on-demand, by the static analyzer engine. >>>>>> >>>>>> CodeInjector.h provides some information, but it is a bit vague: >>>>>> >>>>>> +/// >>>>>> +/// \file >>>>>> +/// \brief Defines the clang::CodeInjector interface which is >>>>>> responsible for >>>>>> +/// injecting AST of function definitions from external source. >>>>>> +/// >>>>>> >>>>>> 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. >>>>>> >>>>> >>>>> I have added some further documentation to address these issues. >>>>> >>>>> >>>>> >>>>>> >>>>>> I'm also looking at the change to >>>>>> StaticAnalyzer/Frontend/FrontendActions.cpp, and wonder if we can >>>>>> simplify >>>>>> things: >>>>>> >>>>>> +++ lib/StaticAnalyzer/Frontend/FrontendActions.cpp (working copy) >>>>>> @@ -7,9 +7,11 @@ >>>>>> // >>>>>> >>>>>> >>>>>> //===----------------------------------------------------------------------===// >>>>>> >>>>>> +#include "clang/Frontend/CompilerInstance.h" >>>>>> #include "clang/StaticAnalyzer/Frontend/FrontendActions.h" >>>>>> -#include "clang/Frontend/CompilerInstance.h" >>>>>> #include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h" >>>>>> +#include "clang/StaticAnalyzer/Frontend/ModelConsumer.h" >>>>>> +#include "ModelInjector.h" >>>>>> using namespace clang; >>>>>> using namespace ento; >>>>>> >>>>>> @@ -18,6 +20,14 @@ >>>>>> return CreateAnalysisConsumer(CI.getPreprocessor(), >>>>>> CI.getFrontendOpts().OutputFile, >>>>>> CI.getAnalyzerOpts(), >>>>>> - CI.getFrontendOpts().Plugins); >>>>>> + CI.getFrontendOpts().Plugins, >>>>>> + new ModelInjector(CI)); >>>>>> } >>>>>> >>>>>> >>>>>> >>>>>> 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: >>>>>> >>>>>> return CreateAnalysisConsumer(CI); >>>>>> >>>>>> and let CreateAnalysisConsumer() do all that boilerplate. >>>>>> >>>>> >>>>> 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. >>>>> >>>>> >>>>>> Next, let's look at the change to FrontendAction: >>>>>> >>>>>> class FrontendAction { >>>>>> + /// Is this action invoked on a model file? Model files are >>>>>> incomplete >>>>>> + /// translation units that relies on type information from another >>>>>> translation >>>>>> + /// unit. Check ParseModelFileAction for details. >>>>>> + bool ModelFile; >>>>>> >>>>>> >>>>>> Perhaps "IsModelFile"? "ModelFile" sounds like it should be a >>>>>> reference to the file itself. >>>>>> >>>>>> FrontendInputFile CurrentInput; >>>>>> std::unique_ptr<ASTUnit> CurrentASTUnit; >>>>>> CompilerInstance *Instance; >>>>>> @@ -105,7 +109,11 @@ >>>>>> /// @} >>>>>> >>>>>> public: >>>>>> - FrontendAction(); >>>>>> + /// \brief Constructor >>>>>> + /// >>>>>> + /// \param modelFile determines whether the source files this >>>>>> action invoked >>>>>> + /// on should be treated as a model file. Defaults to false. >>>>>> + FrontendAction(bool modelFile = false); >>>>>> >>>>>> >>>>>> 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: >>>>>> >>>>>> ParseModelFileAction::ParseModelFileAction(llvm::StringMap<Stmt *> >>>>>> &Bodies) >>>>>> : ASTFrontendAction(/*ModelFile=*/true), Bodies(Bodies) {} >>>>>> >>>>>> becomes: >>>>>> >>>>>> ParseModelFileAction::ParseModelFileAction(llvm::StringMap<Stmt *> >>>>>> &Bodies) >>>>>> : Bodies(Bodies) { >>>>>> IsModelFile = true; >>>>>> } >>>>>> >>>>>> 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: >>>>>> >>>>>> if (hasSourceManager() && !Act.isModelParsingAction()) >>>>>> >>>>>> 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). >>>>>> >>>>>> >>>>> 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? >>>>> >>>>> >>>>>> As for the test case: >>>>>> >>>>>> +typedef int* intptr; >>>>>> + >>>>>> +void modelled(intptr p); >>>>>> + >>>>>> +int main() { >>>>>> + modelled(0); >>>>>> + return 0; >>>>>> +} >>>>>> >>>>>> >>>>>> 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. >>>>>> >>>>>> 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'). >>>>>> >>>>> >>>>> 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. >>>>> >>>>> >>>>>> >>>>>> As for the model files themselves: >>>>>> >>>>>> Index: test/Analysis/modelled.model >>>>>> =================================================================== >>>>>> --- test/Analysis/modelled.model (revision 0) >>>>>> +++ test/Analysis/modelled.model (working copy) >>>>>> @@ -0,0 +1,3 @@ >>>>>> +void modelled(intptr p) { >>>>>> + ++*p; >>>>>> +} >>>>>> \ No newline at end of file >>>>>> Index: test/Analysis/notzero.model >>>>>> =================================================================== >>>>>> --- test/Analysis/notzero.model (revision 0) >>>>>> +++ test/Analysis/notzero.model (working copy) >>>>>> >>>>>> >>>>>> 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. >>>>>> >>>>> >>>>> I have moved the model files to tests/Inputs/Models. >>>>> >>>>> >>>>>> >>>>>> 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. >>>>>> >>>>>> Cheers, >>>>>> Ted >>>>>> >>>>>> >>>>> Thanks, >>>>> Gábor >>>>> >>>>> >>>>>> >>>>>> On Jul 16, 2014, at 2:45 AM, Gábor Horváth <[email protected]> >>>>>> wrote: >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> On 14 July 2014 19:32, Anna Zaks <[email protected]> wrote: >>>>>> >>>>>>> >>>>>>> On Jul 13, 2014, at 6:11 AM, Gábor Horváth <[email protected]> >>>>>>> wrote: >>>>>>> >>>>>>> Hi Anna, >>>>>>> >>>>>>> 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. >>>>>>> >>>>>>> >>>>>>> On 11 July 2014 18:02, Anna Zaks <[email protected]> wrote: >>>>>>> >>>>>>>> >>>>>>>> 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. >>>>>>>> >>>>>>>> >>>>>>> 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). >>>>>>> >>>>>>> >>>>>>> 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. >>>>>>> >>>>>> >>>>>> 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. >>>>>> >>>>>> Thanks, >>>>>> Gábor >>>>>> >>>>>> >>>>>>> Thanks, >>>>>>> Gábor >>>>>>> <api_modeling.patch><bugreporter.patch> >>>>>>> >>>>>>> >>>>>>> >>>>>> <api_modeling.patch><bugreporter.patch> >>>>>> >>>>>> >>>>>> >>>>> <api_modeling.patch><bugreporter.patch> >>>>> >>>>> >>>>> >>>> <api_modeling.patch><bugreporter.patch> >>>> >>>> >>>> >>> >>> Thanks, >>> Gábor >>> <api_modeling.patch><bugreporter.patch><clangTidy.patch> >>> >>> >>> >> <api_modeling.patch><bugreporter.patch><clangTidy.patch> >> >> >> > <api_modeling.patch> > > <bugreporter.patch> > > <clangTidy.patch> > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
