[clang-tools-extra] r219212 - Make clang-tidy's runCheckOnCode actually use the DiagnosticConsumer.
Manuel Klimek
klimek at google.com
Wed Oct 8 11:03:06 PDT 2014
On Wed Oct 08 2014 at 7:54:25 PM David Blaikie <dblaikie at gmail.com> wrote:
> On Wed, Oct 8, 2014 at 10:36 AM, Manuel Klimek <klimek at google.com> wrote:
>
>>
>>
>> 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'm sort of inclined to agree, though I haven't quite tried to fight that
> fight on any API updates I've made so far - feel free to pitch in once I
> add the ownership discussion to cfe-dev.
>
>
>> I remember that ToolInvocation ended up that way because I tried to solve
>> another puzzle related to ownership down the stack.
>>
>
> Hmm? The ToolInvocation one looks actually fairly simple. One ctor doesn't
> take ownership (because the user provides a pointer to a ToolAction that
> the user will own) the other takes owenrship (because you provide a
> FrontendAction that will be returned from the SingleFrontendActionFactory
> that's created internally - thus it must be owned internally).
>
> This is the semi-common idiom for conditional ownership, though in some
> cases in reverse: a default value or a user-provided value. (eg: in some
> cases it's a "well, either I build a stream, or I use stdin - if I built it
> I need to own it, if I use stdin I don't")
>
Usually I would argue strongly for "make the user own and inject whatever
you need". I'm not sure any more what drove that exact decision back in the
day, but I'm not one to make that lightly (I think it's a pain), so I
probably thought this will make the using code significantly simpler.
>
>>
>>
>>> 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/a9d2c3dd/attachment.html>
More information about the cfe-commits
mailing list