[clang-tools-extra] r219212 - Make clang-tidy's runCheckOnCode actually use the DiagnosticConsumer.
David Blaikie
dblaikie at gmail.com
Wed Oct 8 09:57:21 PDT 2014
On Wed, Oct 8, 2014 at 3:10 AM, Alexander Kornienko <alexfh at google.com>
wrote:
> 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."
>
I don't think that's the case - I believe you can singly/externally own a
RefCountedBase<> object. Just don't mix unique_ptr or direct value
instances with IntrusiveRefCntPtr ownership to the same entity.
And that's the problem here - go a few layers down
(ToolInvocation::ToolInvocation -> ToolInvocation::runInvocation ->
FrontendActionFactory::runInvocation -> CompilerInstance::setFileManager),
and CompilerInstance intrusively takes shared ownership of this object.
Now, granted, the CompilerInstance will be destroyed when that function
returns, so its ownership is strictly dominated by the ownership of the
'files' variable in Manuel's code...
But this is one of those ownership puzzles I keep coming across (one flavor
of it anyway) where it might be nice for a caller to say "no, it's OK, I've
got this, you don't need to take ownership". (& in several APIs we have
this kind of conditional ownership - take ToolInvocations handling of the
Action+OwnsAction flag, it'd be possible for CompilerInstance to do a
similar thing - but I'm not sure it's the right answer (I've been trying to
have a decent discussion about this on llvm-dev... but maybe I need to
bring it to cfe-dev too, since I see this idiom more in Clang than LLVM))
Alternatively, of course - push the ref counting explicitly through the
APIs and at least move to std::shared_ptr without intrusion if possible
(shared_ptr supports intrusion if necessary) even though in this case it's
not necessary.
>
>
>>
>>
>>>
>>>
>>>> + 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
>>>>
>>>
>
>
> _______________________________________________
> 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/793e5a3b/attachment.html>
More information about the cfe-commits
mailing list