[clang-tools-extra] r219212 - Make clang-tidy's runCheckOnCode actually use the DiagnosticConsumer.
David Blaikie
dblaikie at gmail.com
Wed Oct 8 10:54:24 PDT 2014
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")
>
>
>> 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/453d9cff/attachment.html>
More information about the cfe-commits
mailing list