r182864 - Tooling: Call back for both begin and end of sources
Vane, Edwin
edwin.vane at intel.com
Mon Jun 3 08:27:52 PDT 2013
> -----Original Message-----
> From: Manuel Klimek [mailto:klimek at google.com]
> Sent: Monday, June 03, 2013 11:17 AM
> To: Vane, Edwin
> Cc: cfe-commits at cs.uiuc.edu
> Subject: Re: r182864 - Tooling: Call back for both begin and end of sources
>
> On Mon, Jun 3, 2013 at 5:06 PM, Vane, Edwin <edwin.vane at intel.com> 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.
> 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 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.
> Thoughts?
> /Manuel
>
>
>
> > -----Original Message-----
> > From: Manuel Klimek [mailto:klimek at google.com]
> > Sent: Monday, June 03, 2013 8:59 AM
> > To: Vane, Edwin
> > Cc: cfe-commits at cs.uiuc.edu
> > 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
> <edwin.vane at intel.com> 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
> > cfe-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> >
> >
>
>
>
More information about the cfe-commits
mailing list