On Wed Oct 08 2014 at 6:57:22 PM David Blaikie <[email protected]> wrote:
> On Wed, Oct 8, 2014 at 3:10 AM, Alexander Kornienko <[email protected]> > wrote: > >> On Wed, Oct 8, 2014 at 1:45 PM, Manuel Klimek <[email protected]> wrote: >> >>> >>> >>> On Wed Oct 08 2014 at 11:34:02 AM Alexander Kornienko <[email protected]> >>> wrote: >>> >>>> On Tue, Oct 7, 2014 at 7:49 PM, Manuel Klimek <[email protected]> >>>> wrote: >>>> >>>>> Author: klimek >>>>> Date: Tue Oct 7 10:49:36 2014 >>>>> New Revision: 219212 >>>>> >>>>> URL: http://llvm.org/viewvc/llvm-project?rev=219212&view=rev >>>>> Log: >>>>> Make clang-tidy's runCheckOnCode actually use the DiagnosticConsumer. >>>>> >>>>> A precondition of that was to run both the preprocessor checks and AST >>>>> checks from the same FrontendAction, otherwise we'd have needed to >>>>> duplicate all involved objects in order to not have any references to a >>>>> deleted source manager. >>>>> >>>>> Modified: >>>>> clang-tools-extra/trunk/unittests/clang-tidy/ClangTidyTest.h >>>>> clang-tools-extra/trunk/unittests/clang-tidy/LLVMModuleTest.cpp >>>>> >>>>> Modified: clang-tools-extra/trunk/unittests/clang-tidy/ClangTidyTest.h >>>>> URL: >>>>> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clang-tidy/ClangTidyTest.h?rev=219212&r1=219211&r2=219212&view=diff >>>>> >>>>> ============================================================================== >>>>> --- clang-tools-extra/trunk/unittests/clang-tidy/ClangTidyTest.h >>>>> (original) >>>>> +++ clang-tools-extra/trunk/unittests/clang-tidy/ClangTidyTest.h Tue >>>>> Oct 7 10:49:36 2014 >>>>> @@ -22,21 +22,23 @@ namespace clang { >>>>> namespace tidy { >>>>> namespace test { >>>>> >>>>> -class TestPPAction : public PreprocessOnlyAction { >>>>> +class TestClangTidyAction : public ASTFrontendAction { >>>>> public: >>>>> - TestPPAction(ClangTidyCheck &Check, ClangTidyContext *Context) >>>>> - : Check(Check), Context(Context) {} >>>>> + TestClangTidyAction(ClangTidyCheck &Check, >>>>> ast_matchers::MatchFinder &Finder, >>>>> + ClangTidyContext &Context) >>>>> + : Check(Check), Finder(Finder), Context(Context) {} >>>>> >>>>> private: >>>>> - bool BeginSourceFileAction(CompilerInstance &Compiler, >>>>> - llvm::StringRef file_name) override { >>>>> - Context->setSourceManager(&Compiler.getSourceManager()); >>>>> + std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance >>>>> &Compiler, >>>>> + StringRef File) >>>>> override { >>>>> + Context.setSourceManager(&Compiler.getSourceManager()); >>>>> Check.registerPPCallbacks(Compiler); >>>>> - return true; >>>>> + return Finder.newASTConsumer(); >>>>> } >>>>> >>>>> ClangTidyCheck &Check; >>>>> - ClangTidyContext *Context; >>>>> + ast_matchers::MatchFinder &Finder; >>>>> + ClangTidyContext &Context; >>>>> }; >>>>> >>>>> template <typename T> >>>>> @@ -50,19 +52,25 @@ std::string runCheckOnCode(StringRef Cod >>>>> ClangTidyGlobalOptions(), Options)); >>>>> ClangTidyDiagnosticConsumer DiagConsumer(Context); >>>>> T Check("test-check", &Context); >>>>> - std::vector<std::string> ArgCXX11(1, "-std=c++11"); >>>>> - ArgCXX11.insert(ArgCXX11.end(), ExtraArgs.begin(), ExtraArgs.end()); >>>>> - >>>>> - if (!tooling::runToolOnCodeWithArgs(new TestPPAction(Check, >>>>> &Context), Code, >>>>> - ArgCXX11, Filename)) >>>>> - return ""; >>>>> ast_matchers::MatchFinder Finder; >>>>> Check.registerMatchers(&Finder); >>>>> - std::unique_ptr<tooling::FrontendActionFactory> Factory( >>>>> - tooling::newFrontendActionFactory(&Finder)); >>>>> - if (!tooling::runToolOnCodeWithArgs(Factory->create(), Code, >>>>> ArgCXX11, >>>>> - Filename)) >>>>> + >>>>> + std::vector<std::string> ArgCXX11(1, "clang-tidy"); >>>>> + ArgCXX11.push_back("-fsyntax-only"); >>>>> + ArgCXX11.push_back("-std=c++11"); >>>>> + ArgCXX11.insert(ArgCXX11.end(), ExtraArgs.begin(), ExtraArgs.end()); >>>>> + ArgCXX11.push_back(Filename.str()); >>>>> + llvm::IntrusiveRefCntPtr<FileManager> files( >>>>> >>>> >>>> Please capitalize "files". >>>> >>> >>> Willdo. >>> >>> >>>> Also, why do we use llvm::IntrusiveRefCntPtr rather than >>>> std::unique_ptr? (I guess there's a reason, but I don't see it.) >>>> >>> >>> This is the same code as in Tooling. I'd like to share that again at >>> some point, but didn't see a good way to resolve this now - if you have >>> ideas, I'm all ears. >>> >> >> Answering my own question: it can't be unique_ptr, because "invoking >> "delete" ... on such [derived from RefCountedBase<>] objects is an error." >> > > I don't think that's the case - I believe you can singly/externally own a > RefCountedBase<> object. Just don't mix unique_ptr or direct value > instances with IntrusiveRefCntPtr ownership to the same entity. > > And that's the problem here - go a few layers down > (ToolInvocation::ToolInvocation -> ToolInvocation::runInvocation -> > FrontendActionFactory::runInvocation -> CompilerInstance::setFileManager), > and CompilerInstance intrusively takes shared ownership of this object. > Now, granted, the CompilerInstance will be destroyed when that function > returns, so its ownership is strictly dominated by the ownership of the > 'files' variable in Manuel's code... > > But this is one of those ownership puzzles I keep coming across (one > flavor of it anyway) where it might be nice for a caller to say "no, it's > OK, I've got this, you don't need to take ownership". (& in several APIs we > have this kind of conditional ownership - take ToolInvocations handling of > the Action+OwnsAction flag, it'd be possible for CompilerInstance to do a > similar thing - but I'm not sure it's the right answer (I've been trying to > have a decent discussion about this on llvm-dev... but maybe I need to > bring it to cfe-dev too, since I see this idiom more in Clang than LLVM)) > I personally think that if there's a chance that a user of the class wants to keep ownership, the class should not take ownership. I remember that ToolInvocation ended up that way because I tried to solve another puzzle related to ownership down the stack. > Alternatively, of course - push the ref counting explicitly through the > APIs and at least move to std::shared_ptr without intrusion if possible > (shared_ptr supports intrusion if necessary) even though in this case it's > not necessary. > > >> >> >>> >>> >>>> >>>> >>>>> + new FileManager(FileSystemOptions())); >>>>> + tooling::ToolInvocation Invocation( >>>>> + ArgCXX11, new TestClangTidyAction(Check, Finder, Context), >>>>> files.get()); >>>>> + SmallString<16> FileNameStorage; >>>>> + StringRef FileNameRef = >>>>> Filename.toNullTerminatedStringRef(FileNameStorage); >>>> >>>> + Invocation.mapVirtualFile(FileNameRef, Code); >>>>> >>>> >>>> Maybe Invocation.mapVirtualFile(Filename.str(), Code)? >>>> >>> >>> Yea, you're right - since this is for testing, we can always copy. >>> Again, I think the right way is to make the interface with tooling simpler >>> to use. >>> >> >> Copying of a filename doesn't have many chances of becoming a bottleneck >> ;) >> >> >>> >>>> >>>> >>>>> + Invocation.setDiagnosticConsumer(&DiagConsumer); >>>>> + if (!Invocation.run()) >>>>> return ""; >>>>> + >>>>> DiagConsumer.finish(); >>>>> tooling::Replacements Fixes; >>>>> for (const ClangTidyError &Error : Context.getErrors()) >>>>> >>>>> Modified: >>>>> clang-tools-extra/trunk/unittests/clang-tidy/LLVMModuleTest.cpp >>>>> URL: >>>>> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clang-tidy/LLVMModuleTest.cpp?rev=219212&r1=219211&r2=219212&view=diff >>>>> >>>>> ============================================================================== >>>>> --- clang-tools-extra/trunk/unittests/clang-tidy/LLVMModuleTest.cpp >>>>> (original) >>>>> +++ clang-tools-extra/trunk/unittests/clang-tidy/LLVMModuleTest.cpp >>>>> Tue Oct 7 10:49:36 2014 >>>>> @@ -114,7 +114,7 @@ TEST(LLVMHeaderGuardCheckTest, FixHeader >>>>> "LLVM_ADT_FOO_H\n#endif \\ >>>>> \n// " >>>>> "LLVM_ADT_FOO_H\n", >>>>> "include/llvm/ADT/foo.h", >>>>> - /*ExpectedWarnings=*/0)); >>>>> + /*ExpectedWarnings=*/1)); >>>>> >>>> >>>> What kind of warning started appearing here? About unknown escape >>>> sequence? >>>> >>> >>> Space between \ and EOL. I'm still confused why this warning didn't >>> trigger go through, but the others did... >>> >>> >>>> >>>> "ExpectedWarnings" doesn't help much =\ I'll change this to actual >>>> warning messages some day. >>>> >>> >>> Oh yes - I had to add print statements to figure out what's going on at >>> all - which was a rather annoying experience ;) >>> >>> >>>> >>>> >>>>> >>>>> EXPECT_EQ("#ifndef LLVM_ADT_FOO_H\n#define LLVM_ADT_FOO_H\n#endif >>>>> /* " >>>>> "LLVM_ADT_FOO_H\\ \n FOO */", >>>>> >>>>> >>>>> _______________________________________________ >>>>> cfe-commits mailing list >>>>> [email protected] >>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>>>> >>>> >> >> >> _______________________________________________ >> cfe-commits mailing list >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> >>
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
