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

[Manuel Klimek]

On Wed Oct 08 2014 at 11:34:02 AM Alexander Kornienko <alexfh at google.com>
wrote:

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

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
>> 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/72fdf0e2/attachment.html>