[clang-tools-extra] r219212 - Make clang-tidy's runCheckOnCode actually use the DiagnosticConsumer.

[Alexander Kornienko]

On Tue, Oct 7, 2014 at 7:49 PM, Manuel Klimek <klimek at google.com> 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". Also, why do we use llvm::IntrusiveRefCntPtr
rather than std::unique_ptr? (I guess there's a reason, but I don't see it.)


> +      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)?


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

"ExpectedWarnings" doesn't help much =\ I'll change this to actual warning
messages some day.


>
>    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
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141008/46a3600b/attachment.html>