On Mon, Jun 3, 2013 at 5:27 PM, Vane, Edwin <[email protected]> wrote:
> > > > -----Original Message----- > > From: Manuel Klimek [mailto:[email protected]] > > Sent: Monday, June 03, 2013 11:17 AM > > To: Vane, Edwin > > Cc: [email protected] > > Subject: Re: r182864 - Tooling: Call back for both begin and end of > sources > > > > On Mon, Jun 3, 2013 at 5:06 PM, Vane, Edwin <[email protected]> > wrote: > > > > > > This was the sort of question I had originally: is a begin > callback useful > > enough to be put in Tooling code? Then I thought: if an end callback is > available > > it only makes sense to have a begin callback for symmetry. Otherwise, > one day > > someone will come along and, like me, say: "Here's an end callback but > no begin > > callback but it's the begin callback I really want. Why is it missing?" > > > > > > > > Well, "why is it missing" is easy to answer: the minimal needed solution > to run a > > clang tool is to hook into the end callback. > > What do you mean? Before making the interface change I went searching for > usages of the end callback and found none. I presumed there must be some > third-party code somewhere that needed it. > Ah, right, /me got confused. > > If the callback is a problem I'll remove it and add a custom > > FrontendAction where it's needed in the migrator. > > > > > > I don't think it's harmful (as it's usually not in the "visible" > interface space > > anyway). > > > > One question is whether we need the CompilerInstance in the callback, as > that is > > a kind of coupling that goes beyond interface symmetry ... > > I'm not sure how CompilerInstance figures with symmetry. It's there > because it's provided by the Well, it doesn't figure with symmetry :) > FrontendAction and why would we hide that information? The argument > already has one user: the add-override transform in the migrator to get > access to the Preprocessor object. From what I've read on the lifetime of > CompilerInstance this method of access seemed in-line with design > philosophy. > Well, in general, I'd try to keep interfaces as small as possible, and only hand through more dependencies when needed. It's not that lifetime is a problem, but that as a user of that interface I would ask myself "why do I get the compiler instance here?", which means a user has to understand that, possibly look into the interface, etc. If you already have a place where it's used, that's fine... > > > Thoughts? > > /Manuel > > > > > > > > > -----Original Message----- > > > From: Manuel Klimek [mailto:[email protected]] > > > Sent: Monday, June 03, 2013 8:59 AM > > > To: Vane, Edwin > > > Cc: [email protected] > > > Subject: Re: r182864 - Tooling: Call back for both begin and end > of > > sources > > > > > > Hmm... My main concern is whether this is really common enough > that > > we don't > > > want to just write FrontendActions for where we need it (it's > not hard > > to write > > > your own FrontendAction - the methods in tooling are mainly > there to > > make the > > > very common use cases less code to write). > > > > > > > > > On Wed, May 29, 2013 at 6:01 PM, Edwin Vane > > <[email protected]> wrote: > > > > > > > > > Author: revane > > > Date: Wed May 29 11:01:10 2013 > > > New Revision: 182864 > > > > > > URL: > http://llvm.org/viewvc/llvm-project?rev=182864&view=rev > > > Log: > > > Tooling: Call back for both begin and end of sources > > > > > > newFrontendActionFactory() took a pointer to a callback to > call > > when a > > > source > > > file was done being processed by an action. This revision > updates > > the > > > callback > > > to include an ante-processing callback as well. > > > > > > Callback-providing class renamed and callback functions > > themselves > > > renamed. > > > Functions are no longer pure-virtual so users aren't > forced to > > implement > > > both > > > callbacks if one isn't needed. > > > > > > > > > Modified: > > > cfe/trunk/include/clang/Tooling/Tooling.h > > > cfe/trunk/unittests/Tooling/ToolingTest.cpp > > > > > > Modified: cfe/trunk/include/clang/Tooling/Tooling.h > > > URL: http://llvm.org/viewvc/llvm- > > > > > project/cfe/trunk/include/clang/Tooling/Tooling.h?rev=182864&r1=182863&r2 > > > =182864&view=diff > > > > > ========================================================== > > > ==================== > > > --- cfe/trunk/include/clang/Tooling/Tooling.h (original) > > > +++ cfe/trunk/include/clang/Tooling/Tooling.h Wed May 29 > > 11:01:10 > > > 2013 > > > @@ -74,12 +74,22 @@ public: > > > template <typename T> > > > FrontendActionFactory *newFrontendActionFactory(); > > > > > > -/// \brief Called at the end of each source file when > used with > > > -/// \c newFrontendActionFactory. > > > -class EndOfSourceFileCallback { > > > +/// \brief Callbacks called before and after each source > file > > processed > > > by a > > > +/// FrontendAction created by the FrontedActionFactory > returned > > by \c > > > +/// newFrontendActionFactory. > > > +class SourceFileCallbacks { > > > public: > > > - virtual ~EndOfSourceFileCallback() {} > > > - virtual void run() = 0; > > > + virtual ~SourceFileCallbacks() {} > > > + > > > + /// \brief Called before a source file is processed by a > > FrontEndAction. > > > > > > > > > > > > FrontendAction. > > > > > > > > > + /// \see clang::FrontendAction::BeginSourceFileAction > > > + virtual bool BeginSource(CompilerInstance &CI, StringRef > > Filename) { > > > + return true; > > > + } > > > + > > > + /// \brief Called after a source file is processed by a > > FrontendAction. > > > + /// \see clang::FrontendAction::EndSourceFileAction > > > + virtual void EndSource() {} > > > }; > > > > > > /// \brief Returns a new FrontendActionFactory for any > type that > > > provides an > > > @@ -95,7 +105,7 @@ public: > > > /// newFrontendActionFactory(&Factory); > > > template <typename FactoryT> > > > inline FrontendActionFactory *newFrontendActionFactory( > > > - FactoryT *ConsumerFactory, EndOfSourceFileCallback > > *EndCallback > > > = NULL); > > > + FactoryT *ConsumerFactory, SourceFileCallbacks > *Callbacks = > > > NULL); > > > > > > /// \brief Runs (and deletes) the tool on 'Code' with the > -fsyntax- > > only > > > flag. > > > /// > > > @@ -226,23 +236,23 @@ FrontendActionFactory > > *newFrontendAction > > > > > > template <typename FactoryT> > > > inline FrontendActionFactory *newFrontendActionFactory( > > > - FactoryT *ConsumerFactory, EndOfSourceFileCallback > > *EndCallback) > > > { > > > + FactoryT *ConsumerFactory, SourceFileCallbacks > *Callbacks) { > > > class FrontendActionFactoryAdapter : public > > FrontendActionFactory { > > > public: > > > explicit FrontendActionFactoryAdapter(FactoryT > > *ConsumerFactory, > > > - > EndOfSourceFileCallback *EndCallback) > > > - : ConsumerFactory(ConsumerFactory), > > EndCallback(EndCallback) {} > > > + > SourceFileCallbacks *Callbacks) > > > + : ConsumerFactory(ConsumerFactory), > Callbacks(Callbacks) {} > > > > > > virtual clang::FrontendAction *create() { > > > - return new ConsumerFactoryAdaptor(ConsumerFactory, > > > EndCallback); > > > + return new ConsumerFactoryAdaptor(ConsumerFactory, > > Callbacks); > > > } > > > > > > private: > > > class ConsumerFactoryAdaptor : public > > clang::ASTFrontendAction { > > > public: > > > ConsumerFactoryAdaptor(FactoryT *ConsumerFactory, > > > - EndOfSourceFileCallback > *EndCallback) > > > - : ConsumerFactory(ConsumerFactory), > > EndCallback(EndCallback) {} > > > + SourceFileCallbacks > *Callbacks) > > > + : ConsumerFactory(ConsumerFactory), > Callbacks(Callbacks) {} > > > > > > clang::ASTConsumer > > *CreateASTConsumer(clang::CompilerInstance > > > &, > > > StringRef) { > > > @@ -250,21 +260,29 @@ inline FrontendActionFactory > > *newFronten > > > } > > > > > > protected: > > > - virtual void EndSourceFileAction() { > > > - if (EndCallback != NULL) > > > - EndCallback->run(); > > > + virtual bool BeginSourceFileAction(CompilerInstance > &CI, > > > + StringRef > Filename) LLVM_OVERRIDE { > > > + if > (!clang::ASTFrontendAction::BeginSourceFileAction(CI, > > > Filename)) > > > + return false; > > > + if (Callbacks != NULL) > > > + return Callbacks->BeginSource(CI, Filename); > > > + return true; > > > + } > > > + virtual void EndSourceFileAction() LLVM_OVERRIDE { > > > + if (Callbacks != NULL) > > > + Callbacks->EndSource(); > > > clang::ASTFrontendAction::EndSourceFileAction(); > > > } > > > > > > private: > > > FactoryT *ConsumerFactory; > > > - EndOfSourceFileCallback *EndCallback; > > > + SourceFileCallbacks *Callbacks; > > > }; > > > FactoryT *ConsumerFactory; > > > - EndOfSourceFileCallback *EndCallback; > > > + SourceFileCallbacks *Callbacks; > > > }; > > > > > > - return new FrontendActionFactoryAdapter(ConsumerFactory, > > > EndCallback); > > > + return new FrontendActionFactoryAdapter(ConsumerFactory, > > > Callbacks); > > > } > > > > > > /// \brief Returns the absolute path of \c File, by > prepending it > > with > > > > > > Modified: cfe/trunk/unittests/Tooling/ToolingTest.cpp > > > URL: http://llvm.org/viewvc/llvm- > > > > > > project/cfe/trunk/unittests/Tooling/ToolingTest.cpp?rev=182864&r1=182863&r > > > 2=182864&view=diff > > > > > ========================================================== > > > ==================== > > > --- cfe/trunk/unittests/Tooling/ToolingTest.cpp (original) > > > +++ cfe/trunk/unittests/Tooling/ToolingTest.cpp Wed May 29 > > 11:01:10 > > > 2013 > > > @@ -131,20 +131,26 @@ TEST(ToolInvocation, > > TestMapVirtualFile) > > > EXPECT_TRUE(Invocation.run()); > > > } > > > > > > -struct VerifyEndCallback : public EndOfSourceFileCallback > { > > > - VerifyEndCallback() : Called(0), Matched(false) {} > > > - virtual void run() { > > > - ++Called; > > > +struct VerifyEndCallback : public SourceFileCallbacks { > > > + VerifyEndCallback() : BeginCalled(0), EndCalled(0), > > Matched(false) {} > > > + virtual bool BeginSource(CompilerInstance &CI, > > > + StringRef Filename) > LLVM_OVERRIDE { > > > + ++BeginCalled; > > > + return true; > > > + } > > > + virtual void EndSource() { > > > + ++EndCalled; > > > } > > > ASTConsumer *newASTConsumer() { > > > return new FindTopLevelDeclConsumer(&Matched); > > > } > > > - unsigned Called; > > > + unsigned BeginCalled; > > > + unsigned EndCalled; > > > bool Matched; > > > }; > > > > > > #if !defined(_WIN32) > > > -TEST(newFrontendActionFactory, > InjectsEndOfSourceFileCallback) > > { > > > +TEST(newFrontendActionFactory, > InjectsSourceFileCallbacks) { > > > VerifyEndCallback EndCallback; > > > > > > FixedCompilationDatabase Compilations("/", > > > std::vector<std::string>()); > > > @@ -159,7 +165,8 @@ TEST(newFrontendActionFactory, > InjectsEn > > > Tool.run(newFrontendActionFactory(&EndCallback, > > &EndCallback)); > > > > > > EXPECT_TRUE(EndCallback.Matched); > > > - EXPECT_EQ(2u, EndCallback.Called); > > > + EXPECT_EQ(2u, EndCallback.BeginCalled); > > > + EXPECT_EQ(2u, EndCallback.EndCalled); > > > } > > > #endif > > > > > > > > > > > > _______________________________________________ > > > 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
