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