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. > > >> + 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. > > >> + 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
