On Wed, Oct 8, 2014 at 10:36 AM, Manuel Klimek <[email protected]> wrote:
> > > On Wed Oct 08 2014 at 6:57:22 PM David Blaikie <[email protected]> wrote: > >> On Wed, Oct 8, 2014 at 3:10 AM, Alexander Kornienko <[email protected]> >> wrote: >> >>> On Wed, Oct 8, 2014 at 1:45 PM, Manuel Klimek <[email protected]> wrote: >>> >>>> >>>> >>>> On Wed Oct 08 2014 at 11:34:02 AM Alexander Kornienko < >>>> [email protected]> wrote: >>>> >>>>> On Tue, Oct 7, 2014 at 7:49 PM, Manuel Klimek <[email protected]> >>>>> 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 >>>>>> [email protected] >>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>>>>> >>>>> >>> >>> >>> _______________________________________________ >>> cfe-commits mailing list >>> [email protected] >>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>> >>>
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
