[clang-tools-extra] r219212 - Make clang-tidy's runCheckOnCode actually use the DiagnosticConsumer.
Manuel Klimek
klimek at google.com
Wed Oct 8 10:36:28 PDT 2014
On Wed Oct 08 2014 at 6:57:22 PM David Blaikie <dblaikie at gmail.com> wrote:
> 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))
>
I personally think that if there's a chance that a user of the class wants
to keep ownership, the class should not take ownership.
I remember that ToolInvocation ended up that way because I tried to solve
another puzzle related to ownership down the stack.
> 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/6f8ef785/attachment.html>
More information about the cfe-commits
mailing list