r182864 - Tooling: Call back for both begin and end of sources
Manuel Klimek
klimek at google.com
Mon Jun 3 09:38:28 PDT 2013
On Mon, Jun 3, 2013 at 5:27 PM, Vane, Edwin <edwin.vane at intel.com> wrote:
>
>
> > -----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.
>
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: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
> > >
> > >
> >
> >
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130603/ed7977ba/attachment.html>
More information about the cfe-commits
mailing list