[clang-tools-extra] r219212 - Make clang-tidy's runCheckOnCode actually use the DiagnosticConsumer.
Alexander Kornienko
alexfh at google.com
Wed Oct 8 03:10:45 PDT 2014
On Wed, Oct 8, 2014 at 1:45 PM, Manuel Klimek <klimek at google.com> wrote:
>
>
> 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.
>
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
>>> 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/3533f6e8/attachment.html>
More information about the cfe-commits
mailing list