[cfe-dev] Problem with a change on newFrontendActionFactory
David Blaikie
dblaikie at gmail.com
Wed Jun 25 16:25:13 PDT 2014
On Tue, Jun 3, 2014 at 12:44 AM, Manuel Klimek <klimek at google.com> wrote:
> On Tue, Jun 3, 2014 at 12:16 AM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>> On Mon, Jun 2, 2014 at 1:34 AM, Manuel Klimek <klimek at google.com> wrote:
>> > Sorry for the late answer. I had it unread in my inbox and was mulling
>> > over
>> > various aspects to it.
>> > My general problem with the whole thing is that even if your solution is
>> > pareto-better (which I personally don't think, but let's assume), I
>> > don't
>> > think it matters enough to be worth the effort of change.
>> > So, I'm not opposed to interface changes, I'm opposed to interface
>> > changes
>> > that don't provide significant enough value to pay for the cost. I am
>> > also
>> > not sure which changes exactly we're now talking about - I think it
>> > would
>> > help if you could split this CL in to the parts that (I hope) are
>> > non-controversial, so we have solved the actual problem, and the parts
>> > that
>> > are controversial.
>> >
>> > On Wed, May 7, 2014 at 6:07 PM, David Blaikie <dblaikie at gmail.com>
>> > wrote:
>> >>
>> >> On Wed, May 7, 2014 at 4:46 AM, Manuel Klimek <klimek at google.com>
>> >> wrote:
>> >> > On Tue, May 6, 2014 at 6:38 PM, David Blaikie <dblaikie at gmail.com>
>> >> > wrote:
>> >> >>
>> >> >> On Tue, May 6, 2014 at 12:42 AM, Manuel Klimek <klimek at google.com>
>> >> >> wrote:
>> >> >> > On Mon, May 5, 2014 at 10:54 PM, David Blaikie
>> >> >> > <dblaikie at gmail.com>
>> >> >> > wrote:
>> >> >> >>
>> >> >> >> On Mon, May 5, 2014 at 8:42 AM, Manuel Klimek <klimek at google.com>
>> >> >> >> wrote:
>> >> >> >> > On Mon, May 5, 2014 at 5:14 PM, David Blaikie
>> >> >> >> > <dblaikie at gmail.com>
>> >> >> >> > wrote:
>> >> >> >> >>
>> >> >> >> >> On Mon, May 5, 2014 at 3:10 AM, Manuel Klimek
>> >> >> >> >> <klimek at google.com>
>> >> >> >> >> wrote:
>> >> >> >> >> > On Thu, May 1, 2014 at 10:21 PM, Richard Smith
>> >> >> >> >> > <richard at metafoo.co.uk>
>> >> >> >> >> > wrote:
>> >> >> >> >> >>
>> >> >> >> >> >> On Thu, May 1, 2014 at 1:19 PM, Nico Weber
>> >> >> >> >> >> <thakis at chromium.org>
>> >> >> >> >> >> wrote:
>> >> >> >> >> >>>
>> >> >> >> >> >>> On Thu, May 1, 2014 at 1:17 PM, David Blaikie
>> >> >> >> >> >>> <dblaikie at gmail.com>
>> >> >> >> >> >>> wrote:
>> >> >> >> >> >>> > On Thu, May 1, 2014 at 1:12 PM, Richard Smith
>> >> >> >> >> >>> > <richard at metafoo.co.uk>
>> >> >> >> >> >>> > wrote:
>> >> >> >> >> >>> >> On Thu, May 1, 2014 at 12:55 PM, Etienne Ollivier
>> >> >> >> >> >>> >> <eollivier at bsu.edu>
>> >> >> >> >> >>> >> wrote:
>> >> >> >> >> >>> >>>
>> >> >> >> >> >>> >>> Hello,
>> >> >> >> >> >>> >>> I updated my clang repository recently and I an error
>> >> >> >> >> >>> >>> appeared
>> >> >> >> >> >>> >>> that
>> >> >> >> >> >>> >>> was
>> >> >> >> >> >>> >>> not
>> >> >> >> >> >>> >>> there before:
>> >> >> >> >> >>> >>>
>> >> >> >> >> >>> >>> error: no viable conversion from
>> >> >> >> >> >>> >>> 'std::unique_ptr<FrontendActionFactory>'
>> >> >> >> >> >>> >>> to
>> >> >> >> >> >>> >>> 'clang::tooling::ToolAction *'
>> >> >> >> >> >>> >>> return
>> >> >> >> >> >>> >>>
>> >> >> >> >> >>> >>> Tool.run(newFrontendActionFactory<MyPluginASTAction>());
>> >> >> >> >> >>> >>>
>> >> >> >> >> >>> >>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> >> >> >> >> >>> >>>
>> >> >> >> >> >>> >>> It is because newFrontendActionFactory has been
>> >> >> >> >> >>> >>> changed
>> >> >> >> >> >>> >>> to
>> >> >> >> >> >>> >>> work
>> >> >> >> >> >>> >>> with
>> >> >> >> >> >>> >>> std::unique_ptr. So if I change my code to
>> >> >> >> >> >>> >>> return
>> >> >> >> >> >>> >>>
>> >> >> >> >> >>> >>>
>> >> >> >> >> >>> >>> Tool.run(&(*newFrontendActionFactory<MyPluginASTAction>()));
>> >> >> >> >> >>> >>
>> >> >> >> >> >>> >>
>> >> >> >> >> >>> >> You can use .get() rather than the slightly non-obvious
>> >> >> >> >> >>> >> &*.
>> >> >> >> >> >>> >>
>> >> >> >> >> >>> >>>
>> >> >> >> >> >>> >>> it works. The only little problem is that it can be
>> >> >> >> >> >>> >>> confusing
>> >> >> >> >> >>> >>> for
>> >> >> >> >> >>> >>> users
>> >> >> >> >> >>> >>> since is not the way it is written in the
>> >> >> >> >> >>> >>> documentation,
>> >> >> >> >> >>> >>> like
>> >> >> >> >> >>> >>> on
>> >> >> >> >> >>> >>> this
>> >> >> >> >> >>> >>> pages:
>> >> >> >> >> >>> >>> http://clang.llvm.org/docs/LibTooling.html
>> >> >> >> >> >>> >>> http://clang.llvm.org/docs/LibASTMatchersTutorial.html
>> >> >> >> >> >>> >>
>> >> >> >> >> >>> >>
>> >> >> >> >> >>> >> Thanks, I've updated the documentation.
>> >> >> >> >> >>> >
>> >> >> >> >> >>> > I'm trying to understand how the ownership used to
>> >> >> >> >> >>> > work/is
>> >> >> >> >> >>> > meant
>> >> >> >> >> >>> > to
>> >> >> >> >> >>> > work now...
>> >> >> >> >> >>>
>> >> >> >> >> >>> The result of newFrontendActionFactory() used to be
>> >> >> >> >> >>> leaked.
>> >> >> >> >> >>> Now
>> >> >> >> >> >>> it's
>> >> >> >> >> >>> freed at the end-of-statement cleanup of the returned
>> >> >> >> >> >>> (invisible)
>> >> >> >> >> >>> unique_ptr temporary.
>> >> >> >> >> >>
>> >> >> >> >> >>
>> >> >> >> >> >> Why do we need to heap-allocate the FrontendActionFactory
>> >> >> >> >> >> at
>> >> >> >> >> >> all?
>> >> >> >> >> >
>> >> >> >> >> >
>> >> >> >> >> > Technically we don't. There's just some ways to create the
>> >> >> >> >> > FrontendActionFactory via templated factory functions
>> >> >> >> >>
>> >> >> >> >> The current factories don't seem to make dynamic choices (or
>> >> >> >> >> even
>> >> >> >> >> templated choices) about which type to return (I may've missed
>> >> >> >> >> something, though) - and the internal templating could still
>> >> >> >> >> be
>> >> >> >> >> implemented via a ctor template instead, I think.
>> >> >> >> >
>> >> >> >> >
>> >> >> >> > How would it store the pointer to the FactoryT*
>> >> >> >> > ConsumerFactory?
>> >> >> >>
>> >> >> >> I'm not sure I understand - it just takes it as a ctor parameter
>> >> >> >> and
>> >> >> >> stores it, doesn't it? Same as when the factory function is used.
>> >> >> >
>> >> >> >
>> >> >> > Sure, but then we a templated constructor wouldn't be enough, we'd
>> >> >> > need
>> >> >> > a
>> >> >> > templated class. If that's what you meant from the start, I
>> >> >> > misunderstood.
>> >> >>
>> >> >> Sorry, yes, I'm not sure what I was trying to say above (I hadn't
>> >> >> looked at the code in detail at that point - so I was probably being
>> >> >> a
>> >> >> bit vague/wrong). But, yes - as you can see in the patches, the
>> >> >> types
>> >> >> can just be used directly.
>> >> >>
>> >> >> >> You talk about needing these things to not move around - so you
>> >> >> >> can
>> >> >> >> point to them - but even that doesn't seem relevant to the
>> >> >> >> construction phase. If these factories returned by value and then
>> >> >> >> that
>> >> >> >> value was passed by const-ref to Tool.run, what would break?
>> >> >> >
>> >> >> >
>> >> >> > Well, we can't return a class with virtual methods by value, can
>> >> >> > we?
>> >> >> > We'd
>> >> >> > need to get rid of the factories if we want to not use pointers
>> >> >> > (and
>> >> >> > you're
>> >> >> > doing that in both patches).
>> >> >>
>> >> >> Not necessarily. The factories could return by value:
>> >> >>
>> >> >> struct base { ... };
>> >> >> struct d1 : base { ... };
>> >> >> struct d2 : base { ... };
>> >> >>
>> >> >> d1 factory() { return d1(); }
>> >> >> d2 factory(int) { return d2(); }
>> >> >>
>> >> >> and usage could either be:
>> >> >>
>> >> >> d1 x = factory();
>> >> >> d2 y = factory(3);
>> >> >>
>> >> >> or:
>> >> >>
>> >> >> base &x = factory();
>> >> >> base &y = factory(3);
>> >> >
>> >> >
>> >> > Having the factory "leak" what concrete object it passes back defeats
>> >> > the
>> >> > purpose of the factory.
>> >>
>> >> Not necessarily - you said one of the benefits was template argument
>> >> deduction and I'm just demonstrating that that can be achieved without
>> >> dynamic allocation.
>> >
>> >
>> > I don't see how that argument gets us any further. My arguments are
>> > always
>> > based on the full context of a specific implementation, not specific
>> > theoretical issues.
>> >
>> >>
>> >> >> But all except 1 caller I touched are just passing the result
>> >> >> straight
>> >> >> to Tool.run, so this change doesn't affect them. The one caller that
>> >> >> did:
>> >> >
>> >> >
>> >> > The problem with an interface intended for use outside of Clang's
>> >> > tree
>> >> > is
>> >> > that I don't think arguing based on what we find in Clang's tree
>> >> > (which
>> >> > are
>> >> > mostly tests) is good enough.
>> >>
>> >> We have 4-5 tools, that seemed like a reasonable bunch of use-cases.
>> >>
>> >> I'm happy to go and look inside Google (where I assume the vast
>> >> majority of tools are so far) and see if we see anything else.
>> >>
>> >> Are there other things you think would be appropriate to use to
>> >> evaluate
>> >> this?
>> >
>> >
>> > Patch it into the internal code-base, make sure all reverse dependency's
>> > tests pass.
>> >
>> >>
>> >>
>> >> >> unique_ptr<factory> p;
>> >> >> if (x)
>> >> >> p = newFactory();
>> >> >> else if (y)
>> >> >> p = newFactory(a);
>> >> >> else
>> >> >> p = newFactory(b);
>> >> >> Tool.run(p);
>> >> >>
>> >> >> and the change was to roll the "Tool.run" call up into the call
>> >> >> site,
>> >> >> which wasn't overly burdensome.
>> >> >>
>> >> >> >> I don't
>> >> >> >> think anything would - the constructor of the
>> >> >> >> FrontendActiorFactories
>> >> >> >> don't appear to leak pointers to themselves. So if you prefer the
>> >> >> >> factory function syntax, you can keep that. The prototype patches
>> >> >> >> attached do not, though (in case you're right about
>> >> >> >> immovability).
>> >> >> >>
>> >> >> >> > Sure, a
>> >> >> >> > templated class would work (basically just instantiate the
>> >> >> >> > FrontendActionFactoryAdapter), but the problem is that then
>> >> >> >> > you'd
>> >> >> >> > always
>> >> >> >> > need to specify the template argument.
>> >> >> >>
>> >> >> >> Having to specify the parameter doesn't seem terribly burdensome.
>> >> >> >
>> >> >> > I find having a unique_ptr return not a terrible problem, so I'd
>> >> >> > argue
>> >> >> > it's
>> >> >> > trade-offs.
>> >> >>
>> >> >> Sure (though as shown above, this particular issue doesn't have to
>> >> >> be
>> >> >> traded off - we can still use function templates as builders and get
>> >> >> the convenience of template argument deduction if it's important)
>> >> >>
>> >> >> >> > Also, there is an overload
>> >> >> >> > newFrontendActionFactory<FrontendActionType>(),
>> >> >> >> > and I think it has value that those form similar patterns.
>> >> >> >>
>> >> >> >> Which would be similarly owned directly in the caller and passed
>> >> >> >> by
>> >> >> >> reference.
>> >> >> >>
>> >> >> >>
>> >> >> >>
>> >> >> >> Tool.run(SimpleFrontendActionFactory<clang::ento::AnalysisAction>());
>> >> >> >>
>> >> >> >> Various cleanup/modification was required, including making
>> >> >> >> FrontendActionFactory's functions const (so the temporary could
>> >> >> >> be
>> >> >> >> passed by const ref) - all existing implementations except the
>> >> >> >> weird
>> >> >> >> SingleFrontendActionFactory, required no changes. (well,
>> >> >> >> technically
>> >> >> >> SingleFrontendActionFactory would've required no changes if I
>> >> >> >> hadn't
>> >> >> >> fixed its raw pointer ownership to be a unique_ptr ownership -
>> >> >> >> then
>> >> >> >> the unique_ptr had to be mutable)
>> >> >> >>
>> >> >> >> It should be possible to remove the virtual dtor from
>> >> >> >> FrontendActionFactory hierarchy too, since it's never
>> >> >> >> polymorphically
>> >> >> >> destroyed, only polymorphically used.
>> >> >> >
>> >> >> >
>> >> >> > I would strongly vote against removing something because "it's not
>> >> >> > polymorphically destroyed", where we expect users of the library
>> >> >> > to
>> >> >> > own
>> >> >> > instances of it. It's very easy to introduce a place where it's
>> >> >> > polymorphically destroyed.
>> >> >>
>> >> >> The same would be true of any type - we don't put virtual dtors on
>> >> >> all
>> >> >> types in case someone tries to polymorphically destroy them. Clang
>> >> >> has
>> >> >> warnings (off by default, though, unfortunately
>> >> >> -Wdelete-non-virtual-dtor) that can catch this bug if it's ever
>> >> >> written.
>> >> >
>> >> >
>> >> > The problem is that if the class is to be used / extended by users of
>> >> > a
>> >> > library, you limit the users' choices when you have a class that is
>> >> > intended
>> >> > to be inherited from, but doesn't have a virtual destructor. If clang
>> >> > was
>> >> > only a program, and not a library, I'd basically agree with most
>> >> > points
>> >> > you
>> >> > make. Since it is a library, I put a lot of emphasis on what
>> >> > interface a
>> >> > potential user gets, how easy it is to write bugs against that
>> >> > interface
>> >> > and
>> >> > understand that interface, and how much flexibility the user gets to
>> >> > design
>> >> > the software the way they want to.
>> >> >
>> >> > That means, in a library that is exposed to users:
>> >> > - a class that's meant to be inherited from by users should have a
>> >> > virtual
>> >> > dtor
>> >>
>> >> While I realize the Tooling API is more API than most of Clang/LLVM
>> >> (which is all intended to be a library and is used as such to varying
>> >> degrees) this isn't necessarily the prevailing attitude. It's come up
>> >> a few times that we've wanted to remove virtual dtors from
>> >> non-polymorphicalyl owned types but currently warnings stop us from
>> >> doing that (-Wvirtual-dtor is on, in part because Google has that on
>> >> internally and we didn't have a way to disable that for LLVM without
>> >> disabling it for all projects that use LLVM - we now do, so we can
>> >> probably turn that warning off in favor of -Wdelete-non-virtual-dtor).
>> >> This has been discussed a few times and seems to be a fairly
>> >> unambiguous prevailing attitude.
>> >>
>> >> http://comments.gmane.org/gmane.comp.compilers.llvm.cv, but now you're
>> >> saying you only theoretically want to do that?s/184558
>> >>
>> >>
>> >> http://comments.gmane.org/gmane.comp.compilers.llvm.cvs/184558
>> >>
>> >> Which reminds me, your concern about bugs can be alleviated, I think,
>> >> by making the base dtor protected and the derived classes final.
>> >
>> >
>> > Following the links I don't see arguments about specific cases where we
>> > want
>> > *external* users of an interface to keep polymorphical ownership of a
>> > class.
>> > Because that's my argument, and if you disagree, I'd like to hear an
>> > argument on why you think this is not a useful feature to have for users
>> > of
>> > this specific interface.
>>
>> Because it's not a feature of our library. To me, making the dtor
>> virtual is the same as making any other function virtual. You do it
>> because it's intended to be overriden by users for use cases you have,
>> practically speaking, use cases of your library. Our library never
>> owns these things so I wouldn't make the dtor virtual.
>
>
> The problem is that other methods don't limit what users of the library can
> do with their own objects,
It does to a degree if we put a non-virtual function in a base class
(or simply don't include a function (even a pure virtual one) in a
base class that a user would like) then the user can't use our
interface to invoke those functions polymorphically. If the user wants
to dynamically own their things, they can introduce an immediate
derived class of FrontendActionFactory and give their derived class a
virtual function - then they can own their own factories dynamically.
They still can't own the default factories dynamically (just as they
can't own vector/list/etc polymorphically).
In any case, the first set of patches doesn't actually remove any
virtual dtors, it just changes the factories, factories' factory
functions, and common usage to not /require/ polymorphic ownership.
I'll look at follow up patches to see about protecting and
devirtualizing the dtor, etc.
> but the destructor does. We intend users to
> implement an interface, so taking the possibility away from them to do
> ownership the way they like it seems like a rather radical move.
>
> I am with you for classes where we actually want to take ownership (let's
> assume we'd have a method for users to inject their own AST nodes, but we
> want to take ownership of the nodes they give us). In that case, I'd also
> vote for making the destructor protected.
Hmm, not sure I follow the case your describing here. If we took
ownership based on some base class, we'd need the base class dtor to
be public (or befriended) and virtual.
>> >> > - classes and methods shouldn't take ownership, unless they are
>> >> > basically
>> >> > glorified containers
>> >>
>> >> The ownership issues with runTool functions I'm not passing judgment
>> >> on - it was merely a mechanical change to reflect the reality of the
>> >> API as it stands today. If that reality can be changed/fixed, I'm OK
>> >> with that.
>> >>
>> >> >> > Because of that I think having virtual functions
>> >> >> > but not having a virtual destructor is an anti-pattern.
>> >> >>
>> >> >> I don't agree - there are lots of types that are generally not
>> >> >> polymorphically owned but are polymorphically used in idioms just
>> >> >> like
>> >> >> this one. Polymorphic usage doesn't imply polymorphic
>> >> >> ownership/destruction.
>> >> >
>> >> >
>> >> > No, it doesn't - but we have style rules because with C++ without
>> >> > rules
>> >> > it
>> >> > is so easy to shoot your foot off that even compiler engineers don't
>> >> > use
>> >> > it
>> >> > that way... Having a rule to always add a virtual dtor if a class
>> >> > has
>> >> > at
>> >> > least one virtual function makes reasoning about code way easier, and
>> >> > not
>> >> > writing a bug is always the least costly variant (even when the
>> >> > compiler
>> >> > can
>> >> > catch it).
>> >>
>> >> >> > My main problem with the attached patches is actually that it
>> >> >> > looks
>> >> >> > to
>> >> >> > me
>> >> >> > like they change ownership in the runTool* functions (if I'm not
>> >> >> > missing
>> >> >> > something).
>> >> >>
>> >> >> Strangely enough, I didn't change the ownership of those functions -
>> >> >> that API change just reflects the reality of the API. Notice
>> >> >> existing
>> >> >> callers (before my patch) passed "Factory.create()" (which returns a
>> >> >> pointer the caller should take ownership of - most of the
>> >> >> implementations are of the form "return new ...") and the
>> >> >
>> >> >
>> >> > Yes, that was an ownership bug in the existing use cases.
>> >>
>> >> 'was'? Has it been fixed? (I don't see the fix)
>> >>
>> >> And where's the bug, exactly? (I just reasoned about the ownership
>> >> semantics of the code, I haven't tried to understand what the
>> >> /intended/ semantics are)
>> >>
>> >> I assume the Factory.create() functions are intended to return
>> >> ownership (should return std::unique_ptr<>).
>> >
>> >
>> > Yes.
>> >
>> >>
>> >> Or are you thinking that
>> >> Factory.create() could return some other kind of RAII wrapper that
>> >> actually returns ownership to the Factory so that a singleton factory
>> >> could be used more than once as long as each use was non-overlapping?
>> >>
>> >> >> implementation agrees in a somewhat circular way: runToolOnCode
>> >> >> creates a ToolInvocation, which builds a SingleFrontendActionFactory
>> >> >> which returns the FAction from its 'create()' function... which
>> >> >> returns ownership.
>> >> >>
>> >> >>
>> >> >> At least that's my understanding of the ownership model. (other
>> >> >> evidence that create() returns ownership - see Tooling.cpp:259,
>> >> >> taking
>> >> >> the result of create() and initializing a unique_ptr with it)
>> >> >
>> >> >
>> >> > Sure, but that means create() returns ownership, not that runTool*
>> >> > should
>> >> > take it. I can see the reasons why we would want them to take
>> >> > ownership,
>> >>
>> >> If create returns ownership, how can runTool* not take ownership?
>> >
>> >
>> > std::unique_ptr<FrontendAction> Action(Factory->Create());
>> > runToolOnCode(Action.get(), "class X {};", ...);
>> >
>> > To me this would be the cleanest way to express ownership.
>>
>> I'm not sure what you're implying here, but if that code change were
>> made today it would lead to a double delete. runToolOnCode takes
>> ownership (because it takes the parameter and, ultimately, returns it
>> from FrontendActionFactory::create, which returns ownership). Are you
>> implying some mechanism that would lead to runToolOnCode not taking
>> ownership of its parameter? How?
>
>
> Ah, you're right, I missed that. We could of course wrap the FrontendAction
> (and forward the calls), but that seems like a bad solution. I take back
> everything I said and claim the opposite ;) (well, not everything...)
If a FrontendAction could be wrapped and reused/returned from multiple
create() calls, we wouldn't need a factory - we could just use a
single FrontendAction all the time. That'd simplify things a bit.
>> I don't know this code very well, I was simply propagating the
>> ownership semantics that were present in the code, and making them
>> explicit in the type system. The code change I suggested reflects the
>> existing ownership semantics of the code.
>>
>> Either we should commit what I've suggested (enshrining the existing
>> ownership semantics into the type system, making them more obvious) or
>> we need to change the semantics. I don't know what kind of change in
>> semantics would be appropriate, but I'm open to
>> suggestions/discussion...
>>
>> >
>> >>
>> >> In
>> >> any case, like I said - I was just changing the API to match the
>> >> semantics as they stand today. This is already the API contract - and
>> >> correct callers are already meeting this contract by passing a
>> >> 'release()'d unique_ptr to runTool* functions. Changing it to
>> >> unique_ptr just makes that more explicit/clear/less error-prone. If
>> >> there's some other API design that fixes this in some other way, I'm
>> >> OK with that - don't really mind.
>> >>
>> >> > but
>> >> > I still am torn, because I value the idea that you can use the same
>> >> > Action
>> >> > for multiple runTool* calls.
>> >> >
>> >> > The original question was about the FrontendActionFactory objects
>> >> > though
>> >> > -
>> >> > here I am still convinced that our interfaces should not take
>> >> > ownership.
>> >>
>> >> I'm confused - I haven't been advocating for FrontendActionFactories
>> >> to have ownership passed to Tool.run. My change only changed Tool.run
>> >> from taking a non-owning pointer to a non-owning reference, in both
>> >> cases the callee owns the factory.
>> >>
>> >> My patch was just to demonstrate an answer to Richard Smith's question:
>> >>
>> >> "Why do we need to heap-allocate the FrontendActionFactory at all?"
>> >>
>> >> (which is to say, by way of example, "we don't need to heap-allocate
>> >> the FrontendActionFactory at all")
>> >
>> >
>> > well, "for the use cases that are in the clang codebase". To me,
>> > interfaces
>> > are about making simple things simple for users. If you can patch it
>> > into
>> > our codebase and make the reverse deps work without making the client
>> > code
>> > significantly more complex, I'm not opposed to the change (but we might
>> > argue about what "more complex" means ...)
>>
>> Working on it.
>
>
> Cool, looking forward to taking a look at the change. Thanks!
Attached the latest patches that preserve the factory factory
functions. The reason I asked about those is that they seem almost a
bit excessive in a world where they're not polymorphically
owning/allocated. Especially the SimpleFrontendActionFactory - why
would that need a factory function to build it? So I figured maybe
it'd be nice to drop the newFrontendActionFactory functions in the
process, if that's the appropriate design currently (even if it means
more churn) rather than keeping a backwards compatible-ish API for the
sake of lower churn.
But if you feel it's the right design (to keep the factory builder
helper functions) irrespective of the history here, that's your call -
I don't feel strongly about it. I've attached patches that go that
way, just having newFrontendActionFactory functions return by value
instead of by unique_ptr.
Does that look like what you were thinking of? Are you OK with a
single commit, or would you like it broken up in any particular ways?
(I have it in a few stages in git, but they're a bit jumbled/would
take some work to pry them out into nicely modular commits)
(you can see in the internal code review the matching changes internally)
- David
>
>>
>>
>> - David
>>
>> >
>> > Cheers,
>> > /Manuel
>> >
>> >>
>> >>
>> >> - David
>> >>
>> >>
>> >> >
>> >> > Cheers,
>> >> > /Manuel
>> >> >
>> >> >
>> >> >>
>> >> >>
>> >> >> - David
>> >> >>
>> >> >> >
>> >> >> > Cheers,
>> >> >> > /Manuel
>> >> >> >
>> >> >> >>
>> >> >> >> - David
>> >> >> >>
>> >> >> >> >
>> >> >> >> >>
>> >> >> >> >>
>> >> >> >> >> > that we want to work
>> >> >> >> >> > with pointers (because they have identity - we want the same
>> >> >> >> >> > one
>> >> >> >> >> > to
>> >> >> >> >> > be
>> >> >> >> >> > passable to different runs and work with the data).
>> >> >> >> >>
>> >> >> >> >> In terms of pointer identity - the idea would be to create a
>> >> >> >> >> FrontendActionFactory as a direct (rather than unique_ptr)
>> >> >> >> >> local
>> >> >> >> >> variable and pass it to each run call - the object would never
>> >> >> >> >> move
>> >> >> >> >> around, so pointers to it would remain valid throughout its
>> >> >> >> >> lifetime.
>> >> >> >> >>
>> >> >> >> >> Or am I picturing something different from what you have in
>> >> >> >> >> mind?
>> >> >> >> >>
>> >> >> >> >> - David
>> >> >> >> >>
>> >> >> >> >> >
>> >> >> >> >> >>
>> >> >> >> >> >>
>> >> >> >> >> >> _______________________________________________
>> >> >> >> >> >> cfe-dev mailing list
>> >> >> >> >> >> cfe-dev at cs.uiuc.edu
>> >> >> >> >> >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>> >> >> >> >> >>
>> >> >> >> >> >
>> >> >> >> >> >
>> >> >> >> >> > _______________________________________________
>> >> >> >> >> > cfe-dev mailing list
>> >> >> >> >> > cfe-dev at cs.uiuc.edu
>> >> >> >> >> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>> >> >> >> >> >
>> >> >> >> >
>> >> >> >> >
>> >> >> >
>> >> >> >
>> >> >
>> >> >
>> >
>> >
>
>
-------------- next part --------------
diff --git include/clang/Tooling/Refactoring.h include/clang/Tooling/Refactoring.h
index cd2fb9f..3e01a03 100644
--- include/clang/Tooling/Refactoring.h
+++ include/clang/Tooling/Refactoring.h
@@ -212,7 +212,7 @@ public:
/// the results to disk.
///
/// \returns 0 upon success. Non-zero upon failure.
- int runAndSave(FrontendActionFactory *ActionFactory);
+ int runAndSave(const FrontendActionFactory &ActionFactory);
/// \brief Apply all stored replacements to the given Rewriter.
///
diff --git include/clang/Tooling/Tooling.h include/clang/Tooling/Tooling.h
index 769acd3..3784384 100644
--- include/clang/Tooling/Tooling.h
+++ include/clang/Tooling/Tooling.h
@@ -39,6 +39,7 @@
#include "clang/Tooling/CompilationDatabase.h"
#include "llvm/ADT/StringMap.h"
#include "llvm/ADT/Twine.h"
+#include "llvm/ADT/STLExtras.h"
#include <memory>
#include <string>
#include <vector>
@@ -66,7 +67,7 @@ public:
/// \brief Perform an action for an invocation.
virtual bool runInvocation(clang::CompilerInvocation *Invocation,
FileManager *Files,
- DiagnosticConsumer *DiagConsumer) = 0;
+ DiagnosticConsumer *DiagConsumer) const = 0;
};
/// \brief Interface to generate clang::FrontendActions.
@@ -81,27 +82,16 @@ public:
/// \brief Invokes the compiler with a FrontendAction created by create().
bool runInvocation(clang::CompilerInvocation *Invocation, FileManager *Files,
- DiagnosticConsumer *DiagConsumer) override;
+ DiagnosticConsumer *DiagConsumer) const override;
/// \brief Returns a new clang::FrontendAction.
///
/// The caller takes ownership of the returned action.
- virtual clang::FrontendAction *create() = 0;
+ virtual std::unique_ptr<clang::FrontendAction> create() const = 0;
};
-/// \brief Returns a new FrontendActionFactory for a given type.
-///
-/// T must derive from clang::FrontendAction.
-///
-/// Example:
-/// FrontendActionFactory *Factory =
-/// newFrontendActionFactory<clang::SyntaxOnlyAction>();
-template <typename T>
-std::unique_ptr<FrontendActionFactory> newFrontendActionFactory();
-
/// \brief Callbacks called before and after each source file processed by a
-/// FrontendAction created by the FrontedActionFactory returned by \c
-/// newFrontendActionFactory.
+/// FrontendAction created by the FrontedActionFactoryAdapter.
class SourceFileCallbacks {
public:
virtual ~SourceFileCallbacks() {}
@@ -117,21 +107,6 @@ public:
virtual void handleEndSource() {}
};
-/// \brief Returns a new FrontendActionFactory for any type that provides an
-/// implementation of newASTConsumer().
-///
-/// FactoryT must implement: ASTConsumer *newASTConsumer().
-///
-/// Example:
-/// struct ProvidesASTConsumers {
-/// clang::ASTConsumer *newASTConsumer();
-/// } Factory;
-/// std::unique_ptr<FrontendActionFactory> FactoryAdapter(
-/// newFrontendActionFactory(&Factory));
-template <typename FactoryT>
-inline std::unique_ptr<FrontendActionFactory> newFrontendActionFactory(
- FactoryT *ConsumerFactory, SourceFileCallbacks *Callbacks = nullptr);
-
/// \brief Runs (and deletes) the tool on 'Code' with the -fsyntax-only flag.
///
/// \param ToolAction The action to run over the code.
@@ -139,8 +114,8 @@ inline std::unique_ptr<FrontendActionFactory> newFrontendActionFactory(
/// \param FileName The file name which 'Code' will be mapped as.
///
/// \return - True if 'ToolAction' was successfully executed.
-bool runToolOnCode(clang::FrontendAction *ToolAction, const Twine &Code,
- const Twine &FileName = "input.cc");
+bool runToolOnCode(std::unique_ptr<clang::FrontendAction> ToolAction,
+ const Twine &Code, const Twine &FileName = "input.cc");
/// \brief Runs (and deletes) the tool on 'Code' with the -fsyntax-only flag and
/// with additional other flags.
@@ -151,7 +126,8 @@ bool runToolOnCode(clang::FrontendAction *ToolAction, const Twine &Code,
/// \param FileName The file name which 'Code' will be mapped as.
///
/// \return - True if 'ToolAction' was successfully executed.
-bool runToolOnCodeWithArgs(clang::FrontendAction *ToolAction, const Twine &Code,
+bool runToolOnCodeWithArgs(std::unique_ptr<clang::FrontendAction> ToolAction,
+ const Twine &Code,
const std::vector<std::string> &Args,
const Twine &FileName = "input.cc");
@@ -188,16 +164,16 @@ class ToolInvocation {
/// \param FAction The action to be executed. Class takes ownership.
/// \param Files The FileManager used for the execution. Class does not take
/// ownership.
- ToolInvocation(std::vector<std::string> CommandLine, FrontendAction *FAction,
- FileManager *Files);
+ ToolInvocation(std::vector<std::string> CommandLine,
+ std::unique_ptr<FrontendAction> FAction, FileManager *Files);
/// \brief Create a tool invocation.
///
/// \param CommandLine The command line arguments to clang.
/// \param Action The action to be executed.
/// \param Files The FileManager used for the execution.
- ToolInvocation(std::vector<std::string> CommandLine, ToolAction *Action,
- FileManager *Files);
+ ToolInvocation(std::vector<std::string> CommandLine,
+ const ToolAction *Action, FileManager *Files);
~ToolInvocation();
@@ -220,10 +196,10 @@ class ToolInvocation {
bool runInvocation(const char *BinaryName,
clang::driver::Compilation *Compilation,
- clang::CompilerInvocation *Invocation);
+ clang::CompilerInvocation *Invocation) const;
std::vector<std::string> CommandLine;
- ToolAction *Action;
+ const ToolAction *Action;
bool OwnsAction;
FileManager *Files;
// Maps <file name> -> <file content>.
@@ -280,7 +256,7 @@ class ClangTool {
/// Runs an action over all files specified in the command line.
///
/// \param Action Tool action.
- int run(ToolAction *Action);
+ int run(const ToolAction &Action);
/// \brief Create an AST for each file specified in the command line and
/// append them to ASTs.
@@ -304,67 +280,73 @@ class ClangTool {
DiagnosticConsumer *DiagConsumer;
};
+namespace internal {
template <typename T>
-std::unique_ptr<FrontendActionFactory> newFrontendActionFactory() {
- class SimpleFrontendActionFactory : public FrontendActionFactory {
- public:
- clang::FrontendAction *create() override { return new T; }
- };
-
- return std::unique_ptr<FrontendActionFactory>(
- new SimpleFrontendActionFactory);
-}
+struct SimpleFrontendActionFactory : FrontendActionFactory {
+ std::unique_ptr<clang::FrontendAction> create() const override {
+ return llvm::make_unique<T>();
+ }
+};
template <typename FactoryT>
-inline std::unique_ptr<FrontendActionFactory> newFrontendActionFactory(
- FactoryT *ConsumerFactory, SourceFileCallbacks *Callbacks) {
- class FrontendActionFactoryAdapter : public FrontendActionFactory {
- public:
- explicit FrontendActionFactoryAdapter(FactoryT *ConsumerFactory,
- SourceFileCallbacks *Callbacks)
+class FrontendActionFactoryAdapter : public FrontendActionFactory {
+public:
+ explicit FrontendActionFactoryAdapter(
+ FactoryT *ConsumerFactory, SourceFileCallbacks *Callbacks = nullptr)
: ConsumerFactory(ConsumerFactory), Callbacks(Callbacks) {}
- clang::FrontendAction *create() override {
- return new ConsumerFactoryAdaptor(ConsumerFactory, Callbacks);
- }
+ std::unique_ptr<clang::FrontendAction> create() const override {
+ return llvm::make_unique<ConsumerFactoryAdaptor>(ConsumerFactory,
+ Callbacks);
+ }
- private:
- class ConsumerFactoryAdaptor : public clang::ASTFrontendAction {
- public:
- ConsumerFactoryAdaptor(FactoryT *ConsumerFactory,
- SourceFileCallbacks *Callbacks)
+private:
+ class ConsumerFactoryAdaptor : public clang::ASTFrontendAction {
+ public:
+ ConsumerFactoryAdaptor(FactoryT *ConsumerFactory,
+ SourceFileCallbacks *Callbacks)
: ConsumerFactory(ConsumerFactory), Callbacks(Callbacks) {}
- clang::ASTConsumer *CreateASTConsumer(clang::CompilerInstance &,
- StringRef) override {
- return ConsumerFactory->newASTConsumer();
- }
-
- protected:
- bool BeginSourceFileAction(CompilerInstance &CI,
- StringRef Filename) override {
- if (!clang::ASTFrontendAction::BeginSourceFileAction(CI, Filename))
- return false;
- if (Callbacks)
- return Callbacks->handleBeginSource(CI, Filename);
- return true;
- }
- void EndSourceFileAction() override {
- if (Callbacks)
- Callbacks->handleEndSource();
- clang::ASTFrontendAction::EndSourceFileAction();
- }
-
- private:
- FactoryT *ConsumerFactory;
- SourceFileCallbacks *Callbacks;
- };
+ clang::ASTConsumer *CreateASTConsumer(clang::CompilerInstance &,
+ StringRef) override {
+ return ConsumerFactory->newASTConsumer();
+ }
+
+ protected:
+ bool BeginSourceFileAction(CompilerInstance &CI,
+ StringRef Filename) override {
+ if (!clang::ASTFrontendAction::BeginSourceFileAction(CI, Filename))
+ return false;
+ if (Callbacks != NULL)
+ return Callbacks->handleBeginSource(CI, Filename);
+ return true;
+ }
+ void EndSourceFileAction() override {
+ if (Callbacks != NULL)
+ Callbacks->handleEndSource();
+ clang::ASTFrontendAction::EndSourceFileAction();
+ }
+
+ private:
FactoryT *ConsumerFactory;
SourceFileCallbacks *Callbacks;
};
+ FactoryT *ConsumerFactory;
+ SourceFileCallbacks *Callbacks;
+};
+}
- return std::unique_ptr<FrontendActionFactory>(
- new FrontendActionFactoryAdapter(ConsumerFactory, Callbacks));
+template <typename T>
+internal::SimpleFrontendActionFactory<T> newFrontendActionFactory() {
+ return internal::SimpleFrontendActionFactory<T>();
+}
+
+template <typename FactoryT>
+internal::FrontendActionFactoryAdapter<FactoryT>
+newFrontendActionFactory(FactoryT *ConsumerFactory,
+ SourceFileCallbacks *Callbacks = 0) {
+ return internal::FrontendActionFactoryAdapter<FactoryT>(ConsumerFactory,
+ Callbacks);
}
/// \brief Returns the absolute path of \c File, by prepending it with
diff --git lib/Tooling/Refactoring.cpp lib/Tooling/Refactoring.cpp
index c96b8c9..1ca8865 100644
--- lib/Tooling/Refactoring.cpp
+++ lib/Tooling/Refactoring.cpp
@@ -277,7 +277,7 @@ RefactoringTool::RefactoringTool(const CompilationDatabase &Compilations,
Replacements &RefactoringTool::getReplacements() { return Replace; }
-int RefactoringTool::runAndSave(FrontendActionFactory *ActionFactory) {
+int RefactoringTool::runAndSave(const FrontendActionFactory &ActionFactory) {
if (int Result = run(ActionFactory)) {
return Result;
}
diff --git lib/Tooling/Tooling.cpp lib/Tooling/Tooling.cpp
index 5d3de8a..c1ed949 100644
--- lib/Tooling/Tooling.cpp
+++ lib/Tooling/Tooling.cpp
@@ -104,10 +104,10 @@ static clang::CompilerInvocation *newInvocation(
return Invocation;
}
-bool runToolOnCode(clang::FrontendAction *ToolAction, const Twine &Code,
- const Twine &FileName) {
- return runToolOnCodeWithArgs(
- ToolAction, Code, std::vector<std::string>(), FileName);
+bool runToolOnCode(std::unique_ptr<clang::FrontendAction> ToolAction,
+ const Twine &Code, const Twine &FileName) {
+ return runToolOnCodeWithArgs(std::move(ToolAction), Code,
+ std::vector<std::string>(), FileName);
}
static std::vector<std::string>
@@ -121,15 +121,16 @@ getSyntaxOnlyToolArgs(const std::vector<std::string> &ExtraArgs,
return Args;
}
-bool runToolOnCodeWithArgs(clang::FrontendAction *ToolAction, const Twine &Code,
+bool runToolOnCodeWithArgs(std::unique_ptr<clang::FrontendAction> ToolAction,
+ const Twine &Code,
const std::vector<std::string> &Args,
const Twine &FileName) {
SmallString<16> FileNameStorage;
StringRef FileNameRef = FileName.toNullTerminatedStringRef(FileNameStorage);
llvm::IntrusiveRefCntPtr<FileManager> Files(
new FileManager(FileSystemOptions()));
- ToolInvocation Invocation(getSyntaxOnlyToolArgs(Args, FileNameRef), ToolAction,
- Files.getPtr());
+ ToolInvocation Invocation(getSyntaxOnlyToolArgs(Args, FileNameRef),
+ std::move(ToolAction), Files.getPtr());
SmallString<1024> CodeStorage;
Invocation.mapVirtualFile(FileNameRef,
@@ -155,18 +156,21 @@ std::string getAbsolutePath(StringRef File) {
namespace {
class SingleFrontendActionFactory : public FrontendActionFactory {
- FrontendAction *Action;
+ mutable std::unique_ptr<FrontendAction> Action;
public:
- SingleFrontendActionFactory(FrontendAction *Action) : Action(Action) {}
+ SingleFrontendActionFactory(std::unique_ptr<FrontendAction> Action)
+ : Action(std::move(Action)) {}
- FrontendAction *create() override { return Action; }
+ std::unique_ptr<FrontendAction> create() const override {
+ return std::move(Action);
+ }
};
}
ToolInvocation::ToolInvocation(std::vector<std::string> CommandLine,
- ToolAction *Action, FileManager *Files)
+ const ToolAction *Action, FileManager *Files)
: CommandLine(std::move(CommandLine)),
Action(Action),
OwnsAction(false),
@@ -174,9 +178,10 @@ ToolInvocation::ToolInvocation(std::vector<std::string> CommandLine,
DiagConsumer(nullptr) {}
ToolInvocation::ToolInvocation(std::vector<std::string> CommandLine,
- FrontendAction *FAction, FileManager *Files)
+ std::unique_ptr<FrontendAction> FAction,
+ FileManager *Files)
: CommandLine(std::move(CommandLine)),
- Action(new SingleFrontendActionFactory(FAction)),
+ Action(new SingleFrontendActionFactory(std::move(FAction))),
OwnsAction(true),
Files(Files),
DiagConsumer(nullptr) {}
@@ -229,10 +234,10 @@ bool ToolInvocation::run() {
return runInvocation(BinaryName, Compilation.get(), Invocation.release());
}
-bool ToolInvocation::runInvocation(
- const char *BinaryName,
- clang::driver::Compilation *Compilation,
- clang::CompilerInvocation *Invocation) {
+bool
+ToolInvocation::runInvocation(const char *BinaryName,
+ clang::driver::Compilation *Compilation,
+ clang::CompilerInvocation *Invocation) const {
// Show the invocation, with -v.
if (Invocation->getHeaderSearchOpts().Verbose) {
llvm::errs() << "clang Invocation:\n";
@@ -243,9 +248,10 @@ bool ToolInvocation::runInvocation(
return Action->runInvocation(Invocation, Files, DiagConsumer);
}
-bool FrontendActionFactory::runInvocation(CompilerInvocation *Invocation,
- FileManager *Files,
- DiagnosticConsumer *DiagConsumer) {
+bool
+FrontendActionFactory::runInvocation(CompilerInvocation *Invocation,
+ FileManager *Files,
+ DiagnosticConsumer *DiagConsumer) const {
// Create a compiler instance to handle the actual work.
clang::CompilerInstance Compiler;
Compiler.setInvocation(Invocation);
@@ -318,7 +324,7 @@ void ClangTool::clearArgumentsAdjusters() {
ArgsAdjusters.clear();
}
-int ClangTool::run(ToolAction *Action) {
+int ClangTool::run(const ToolAction &Action) {
// Exists solely for the purpose of lookup of the resource path.
// This just needs to be some symbol in the binary.
static int StaticSymbol;
@@ -352,7 +358,7 @@ int ClangTool::run(ToolAction *Action) {
DEBUG({
llvm::dbgs() << "Processing: " << Command.first << ".\n";
});
- ToolInvocation Invocation(std::move(CommandLine), Action, Files.getPtr());
+ ToolInvocation Invocation(std::move(CommandLine), &Action, Files.getPtr());
Invocation.setDiagnosticConsumer(DiagConsumer);
for (const auto &MappedFile : MappedFileContents) {
Invocation.mapVirtualFile(MappedFile.first, MappedFile.second);
@@ -375,7 +381,7 @@ public:
ASTBuilderAction(std::vector<std::unique_ptr<ASTUnit>> &ASTs) : ASTs(ASTs) {}
bool runInvocation(CompilerInvocation *Invocation, FileManager *Files,
- DiagnosticConsumer *DiagConsumer) override {
+ DiagnosticConsumer *DiagConsumer) const override {
// FIXME: This should use the provided FileManager.
std::unique_ptr<ASTUnit> AST = ASTUnit::LoadFromCompilerInvocation(
Invocation, CompilerInstance::createDiagnostics(
@@ -393,7 +399,7 @@ public:
int ClangTool::buildASTs(std::vector<std::unique_ptr<ASTUnit>> &ASTs) {
ASTBuilderAction Action(ASTs);
- return run(&Action);
+ return run(Action);
}
std::unique_ptr<ASTUnit> buildASTFromCode(const Twine &Code,
diff --git tools/clang-check/ClangCheck.cpp tools/clang-check/ClangCheck.cpp
index cc8d43c..beafd87 100644
--- tools/clang-check/ClangCheck.cpp
+++ tools/clang-check/ClangCheck.cpp
@@ -215,15 +215,10 @@ int main(int argc, const char **argv) {
Analyze ? "--analyze" : "-fsyntax-only", InsertAdjuster::BEGIN));
clang_check::ClangCheckActionFactory CheckFactory;
- std::unique_ptr<FrontendActionFactory> FrontendFactory;
-
// Choose the correct factory based on the selected mode.
if (Analyze)
- FrontendFactory = newFrontendActionFactory<clang::ento::AnalysisAction>();
- else if (Fixit)
- FrontendFactory = newFrontendActionFactory<FixItAction>();
- else
- FrontendFactory = newFrontendActionFactory(&CheckFactory);
-
- return Tool.run(FrontendFactory.get());
+ return Tool.run(newFrontendActionFactory<clang::ento::AnalysisAction>());
+ if (Fixit)
+ return Tool.run(newFrontendActionFactory<FixItAction>());
+ return Tool.run(newFrontendActionFactory(&CheckFactory));
}
diff --git unittests/AST/ASTContextParentMapTest.cpp unittests/AST/ASTContextParentMapTest.cpp
index 0dcb175..626d410 100644
--- unittests/AST/ASTContextParentMapTest.cpp
+++ unittests/AST/ASTContextParentMapTest.cpp
@@ -21,7 +21,6 @@
namespace clang {
namespace ast_matchers {
-using clang::tooling::newFrontendActionFactory;
using clang::tooling::runToolOnCodeWithArgs;
using clang::tooling::FrontendActionFactory;
diff --git unittests/AST/DeclPrinterTest.cpp unittests/AST/DeclPrinterTest.cpp
index 5340756..0176884 100644
--- unittests/AST/DeclPrinterTest.cpp
+++ unittests/AST/DeclPrinterTest.cpp
@@ -74,10 +74,9 @@ public:
PrintMatch Printer;
MatchFinder Finder;
Finder.addMatcher(NodeMatch, &Printer);
- std::unique_ptr<FrontendActionFactory> Factory(
- newFrontendActionFactory(&Finder));
+ auto Factory = newFrontendActionFactory(&Finder);
- if (!runToolOnCodeWithArgs(Factory->create(), Code, Args, FileName))
+ if (!runToolOnCodeWithArgs(Factory.create(), Code, Args, FileName))
return testing::AssertionFailure()
<< "Parsing error in \"" << Code.str() << "\"";
diff --git unittests/AST/DeclTest.cpp unittests/AST/DeclTest.cpp
index 87aeef4..cd41430 100644
--- unittests/AST/DeclTest.cpp
+++ unittests/AST/DeclTest.cpp
@@ -20,8 +20,7 @@ using namespace clang::tooling;
TEST(Decl, CleansUpAPValues) {
MatchFinder Finder;
- std::unique_ptr<FrontendActionFactory> Factory(
- newFrontendActionFactory(&Finder));
+ auto Factory = newFrontendActionFactory(&Finder);
// This is a regression test for a memory leak in APValues for structs that
// allocate memory. This test only fails if run under valgrind with full leak
@@ -29,7 +28,7 @@ TEST(Decl, CleansUpAPValues) {
std::vector<std::string> Args(1, "-std=c++11");
Args.push_back("-fno-ms-extensions");
ASSERT_TRUE(runToolOnCodeWithArgs(
- Factory->create(),
+ Factory.create(),
"struct X { int a; }; constexpr X x = { 42 };"
"union Y { constexpr Y(int a) : a(a) {} int a; }; constexpr Y y = { 42 };"
"constexpr int z[2] = { 42, 43 };"
@@ -53,7 +52,6 @@ TEST(Decl, CleansUpAPValues) {
// FIXME: Once this test starts breaking we can test APValue::needsCleanup
// for ComplexInt.
ASSERT_FALSE(runToolOnCodeWithArgs(
- Factory->create(),
- "constexpr _Complex __uint128_t c = 0xffffffffffffffff;",
- Args));
+ Factory.create(),
+ "constexpr _Complex __uint128_t c = 0xffffffffffffffff;", Args));
}
diff --git unittests/AST/EvaluateAsRValueTest.cpp unittests/AST/EvaluateAsRValueTest.cpp
index 9120c93..ae0018a 100644
--- unittests/AST/EvaluateAsRValueTest.cpp
+++ unittests/AST/EvaluateAsRValueTest.cpp
@@ -91,22 +91,22 @@ TEST(EvaluateAsRValue, FailsGracefullyForUnknownTypes) {
std::vector<std::string> Args(1, Mode);
Args.push_back("-fno-delayed-template-parsing");
ASSERT_TRUE(runToolOnCodeWithArgs(
- new EvaluateConstantInitializersAction(),
- "template <typename T>"
- "struct vector {"
- " explicit vector(int size);"
- "};"
- "template <typename R>"
- "struct S {"
- " vector<R> intervals() const {"
- " vector<R> Dependent(2);"
- " return Dependent;"
- " }"
- "};"
- "void doSomething() {"
- " int Constant = 2 + 2;"
- " (void) Constant;"
- "}",
- Args));
+ llvm::make_unique<EvaluateConstantInitializersAction>(),
+ "template <typename T>"
+ "struct vector {"
+ " explicit vector(int size);"
+ "};"
+ "template <typename R>"
+ "struct S {"
+ " vector<R> intervals() const {"
+ " vector<R> Dependent(2);"
+ " return Dependent;"
+ " }"
+ "};"
+ "void doSomething() {"
+ " int Constant = 2 + 2;"
+ " (void) Constant;"
+ "}",
+ Args));
}
}
diff --git unittests/AST/MatchVerifier.h unittests/AST/MatchVerifier.h
index 0265f4a..63dbc99 100644
--- unittests/AST/MatchVerifier.h
+++ unittests/AST/MatchVerifier.h
@@ -79,8 +79,7 @@ testing::AssertionResult MatchVerifier<NodeType>::match(
std::vector<std::string>& Args, Language L) {
MatchFinder Finder;
Finder.addMatcher(AMatcher.bind(""), this);
- std::unique_ptr<tooling::FrontendActionFactory> Factory(
- tooling::newFrontendActionFactory(&Finder));
+ auto Factory = tooling::newFrontendActionFactory(&Finder);
StringRef FileName;
switch (L) {
@@ -106,7 +105,7 @@ testing::AssertionResult MatchVerifier<NodeType>::match(
// Default to failure in case callback is never called
setFailure("Could not find match");
- if (!tooling::runToolOnCodeWithArgs(Factory->create(), Code, Args, FileName))
+ if (!tooling::runToolOnCodeWithArgs(Factory.create(), Code, Args, FileName))
return testing::AssertionFailure() << "Parsing error";
if (!Verified)
return testing::AssertionFailure() << VerifyResult;
diff --git unittests/AST/NamedDeclPrinterTest.cpp unittests/AST/NamedDeclPrinterTest.cpp
index 4823b44..6ab677b 100644
--- unittests/AST/NamedDeclPrinterTest.cpp
+++ unittests/AST/NamedDeclPrinterTest.cpp
@@ -68,10 +68,9 @@ PrintedNamedDeclMatches(StringRef Code, const std::vector<std::string> &Args,
PrintMatch Printer(SuppressUnwrittenScope);
MatchFinder Finder;
Finder.addMatcher(NodeMatch, &Printer);
- std::unique_ptr<FrontendActionFactory> Factory(
- newFrontendActionFactory(&Finder));
+ auto Factory = newFrontendActionFactory(&Finder);
- if (!runToolOnCodeWithArgs(Factory->create(), Code, Args, FileName))
+ if (!runToolOnCodeWithArgs(Factory.create(), Code, Args, FileName))
return testing::AssertionFailure()
<< "Parsing error in \"" << Code.str() << "\"";
diff --git unittests/AST/StmtPrinterTest.cpp unittests/AST/StmtPrinterTest.cpp
index 541fb3d..03698e8 100644
--- unittests/AST/StmtPrinterTest.cpp
+++ unittests/AST/StmtPrinterTest.cpp
@@ -73,10 +73,9 @@ PrintedStmtMatches(StringRef Code, const std::vector<std::string> &Args,
PrintMatch Printer;
MatchFinder Finder;
Finder.addMatcher(NodeMatch, &Printer);
- std::unique_ptr<FrontendActionFactory> Factory(
- newFrontendActionFactory(&Finder));
+ auto Factory = newFrontendActionFactory(&Finder);
- if (!runToolOnCodeWithArgs(Factory->create(), Code, Args))
+ if (!runToolOnCodeWithArgs(Factory.create(), Code, Args))
return testing::AssertionFailure()
<< "Parsing error in \"" << Code.str() << "\"";
diff --git unittests/ASTMatchers/ASTMatchersTest.cpp unittests/ASTMatchers/ASTMatchersTest.cpp
index d247fac..9d1da91 100644
--- unittests/ASTMatchers/ASTMatchersTest.cpp
+++ unittests/ASTMatchers/ASTMatchersTest.cpp
@@ -4237,9 +4237,8 @@ TEST(MatchFinder, InterceptsStartOfTranslationUnit) {
MatchFinder Finder;
VerifyStartOfTranslationUnit VerifyCallback;
Finder.addMatcher(decl(), &VerifyCallback);
- std::unique_ptr<FrontendActionFactory> Factory(
- newFrontendActionFactory(&Finder));
- ASSERT_TRUE(tooling::runToolOnCode(Factory->create(), "int x;"));
+ auto Factory = newFrontendActionFactory(&Finder);
+ ASSERT_TRUE(tooling::runToolOnCode(Factory.create(), "int x;"));
EXPECT_TRUE(VerifyCallback.Called);
VerifyCallback.Called = false;
@@ -4265,9 +4264,8 @@ TEST(MatchFinder, InterceptsEndOfTranslationUnit) {
MatchFinder Finder;
VerifyEndOfTranslationUnit VerifyCallback;
Finder.addMatcher(decl(), &VerifyCallback);
- std::unique_ptr<FrontendActionFactory> Factory(
- newFrontendActionFactory(&Finder));
- ASSERT_TRUE(tooling::runToolOnCode(Factory->create(), "int x;"));
+ auto Factory = newFrontendActionFactory(&Finder);
+ ASSERT_TRUE(tooling::runToolOnCode(Factory.create(), "int x;"));
EXPECT_TRUE(VerifyCallback.Called);
VerifyCallback.Called = false;
diff --git unittests/ASTMatchers/ASTMatchersTest.h unittests/ASTMatchers/ASTMatchersTest.h
index 2e4ee2c..c334c76 100644
--- unittests/ASTMatchers/ASTMatchersTest.h
+++ unittests/ASTMatchers/ASTMatchersTest.h
@@ -69,11 +69,10 @@ testing::AssertionResult matchesConditionally(const std::string &Code,
VerifyMatch VerifyDynamicFound(nullptr, &DynamicFound);
if (!Finder.addDynamicMatcher(AMatcher, &VerifyDynamicFound))
return testing::AssertionFailure() << "Could not add dynamic matcher";
- std::unique_ptr<FrontendActionFactory> Factory(
- newFrontendActionFactory(&Finder));
+ auto Factory = newFrontendActionFactory(&Finder);
// Some tests use typeof, which is a gnu extension.
std::vector<std::string> Args(1, CompileArg);
- if (!runToolOnCodeWithArgs(Factory->create(), Code, Args)) {
+ if (!runToolOnCodeWithArgs(Factory.create(), Code, Args)) {
return testing::AssertionFailure() << "Parsing error in \"" << Code << "\"";
}
if (Found != DynamicFound) {
@@ -113,11 +112,10 @@ matchAndVerifyResultConditionally(const std::string &Code, const T &AMatcher,
MatchFinder Finder;
VerifyMatch VerifyVerifiedResult(FindResultVerifier, &VerifiedResult);
Finder.addMatcher(AMatcher, &VerifyVerifiedResult);
- std::unique_ptr<FrontendActionFactory> Factory(
- newFrontendActionFactory(&Finder));
+ auto Factory = newFrontendActionFactory(&Finder);
// Some tests use typeof, which is a gnu extension.
std::vector<std::string> Args(1, "-std=gnu++98");
- if (!runToolOnCodeWithArgs(Factory->create(), Code, Args)) {
+ if (!runToolOnCodeWithArgs(Factory.create(), Code, Args)) {
return testing::AssertionFailure() << "Parsing error in \"" << Code << "\"";
}
if (!VerifiedResult && ExpectResult) {
diff --git unittests/Sema/ExternalSemaSourceTest.cpp unittests/Sema/ExternalSemaSourceTest.cpp
index bc0d632..16e7d75 100644
--- unittests/Sema/ExternalSemaSourceTest.cpp
+++ unittests/Sema/ExternalSemaSourceTest.cpp
@@ -178,28 +178,26 @@ public:
// Make sure that the NamespaceDiagnosticWatcher is not miscounting.
TEST(ExternalSemaSource, SanityCheck) {
- std::unique_ptr<ExternalSemaSourceInstaller> Installer(
- new ExternalSemaSourceInstaller);
+ auto Installer = llvm::make_unique<ExternalSemaSourceInstaller>();
NamespaceDiagnosticWatcher Watcher("AAB", "BBB");
Installer->PushWatcher(&Watcher);
std::vector<std::string> Args(1, "-std=c++11");
ASSERT_TRUE(clang::tooling::runToolOnCodeWithArgs(
- Installer.release(), "namespace AAA { } using namespace AAB;", Args));
+ std::move(Installer), "namespace AAA { } using namespace AAB;", Args));
ASSERT_EQ(0, Watcher.SeenCount);
}
// Check that when we add a NamespaceTypeProvider, we use that suggestion
// instead of the usual suggestion we would use above.
TEST(ExternalSemaSource, ExternalTypoCorrectionPrioritized) {
- std::unique_ptr<ExternalSemaSourceInstaller> Installer(
- new ExternalSemaSourceInstaller);
+ auto Installer = llvm::make_unique<ExternalSemaSourceInstaller>();
NamespaceTypoProvider Provider("AAB", "BBB");
NamespaceDiagnosticWatcher Watcher("AAB", "BBB");
Installer->PushSource(&Provider);
Installer->PushWatcher(&Watcher);
std::vector<std::string> Args(1, "-std=c++11");
ASSERT_TRUE(clang::tooling::runToolOnCodeWithArgs(
- Installer.release(), "namespace AAA { } using namespace AAB;", Args));
+ std::move(Installer), "namespace AAA { } using namespace AAB;", Args));
ASSERT_LE(0, Provider.CallCount);
ASSERT_EQ(1, Watcher.SeenCount);
}
@@ -207,8 +205,7 @@ TEST(ExternalSemaSource, ExternalTypoCorrectionPrioritized) {
// Check that we use the first successful TypoCorrection returned from an
// ExternalSemaSource.
TEST(ExternalSemaSource, ExternalTypoCorrectionOrdering) {
- std::unique_ptr<ExternalSemaSourceInstaller> Installer(
- new ExternalSemaSourceInstaller);
+ auto Installer = llvm::make_unique<ExternalSemaSourceInstaller>();
NamespaceTypoProvider First("XXX", "BBB");
NamespaceTypoProvider Second("AAB", "CCC");
NamespaceTypoProvider Third("AAB", "DDD");
@@ -219,7 +216,7 @@ TEST(ExternalSemaSource, ExternalTypoCorrectionOrdering) {
Installer->PushWatcher(&Watcher);
std::vector<std::string> Args(1, "-std=c++11");
ASSERT_TRUE(clang::tooling::runToolOnCodeWithArgs(
- Installer.release(), "namespace AAA { } using namespace AAB;", Args));
+ std::move(Installer), "namespace AAA { } using namespace AAB;", Args));
ASSERT_LE(1, First.CallCount);
ASSERT_LE(1, Second.CallCount);
ASSERT_EQ(0, Third.CallCount);
@@ -229,15 +226,14 @@ TEST(ExternalSemaSource, ExternalTypoCorrectionOrdering) {
// We should only try MaybeDiagnoseMissingCompleteType if we can't otherwise
// solve the problem.
TEST(ExternalSemaSource, TryOtherTacticsBeforeDiagnosing) {
- std::unique_ptr<ExternalSemaSourceInstaller> Installer(
- new ExternalSemaSourceInstaller);
+ auto Installer = llvm::make_unique<ExternalSemaSourceInstaller>();
CompleteTypeDiagnoser Diagnoser(false);
Installer->PushSource(&Diagnoser);
std::vector<std::string> Args(1, "-std=c++11");
// This code hits the class template specialization/class member of a class
// template specialization checks in Sema::RequireCompleteTypeImpl.
ASSERT_TRUE(clang::tooling::runToolOnCodeWithArgs(
- Installer.release(),
+ std::move(Installer),
"template <typename T> struct S { class C { }; }; S<char>::C SCInst;",
Args));
ASSERT_EQ(0, Diagnoser.CallCount);
@@ -246,8 +242,7 @@ TEST(ExternalSemaSource, TryOtherTacticsBeforeDiagnosing) {
// The first ExternalSemaSource where MaybeDiagnoseMissingCompleteType returns
// true should be the last one called.
TEST(ExternalSemaSource, FirstDiagnoserTaken) {
- std::unique_ptr<ExternalSemaSourceInstaller> Installer(
- new ExternalSemaSourceInstaller);
+ auto Installer = llvm::make_unique<ExternalSemaSourceInstaller>();
CompleteTypeDiagnoser First(false);
CompleteTypeDiagnoser Second(true);
CompleteTypeDiagnoser Third(true);
@@ -256,7 +251,7 @@ TEST(ExternalSemaSource, FirstDiagnoserTaken) {
Installer->PushSource(&Third);
std::vector<std::string> Args(1, "-std=c++11");
ASSERT_FALSE(clang::tooling::runToolOnCodeWithArgs(
- Installer.release(), "class Incomplete; Incomplete IncompleteInstance;",
+ std::move(Installer), "class Incomplete; Incomplete IncompleteInstance;",
Args));
ASSERT_EQ(1, First.CallCount);
ASSERT_EQ(1, Second.CallCount);
diff --git unittests/Tooling/CommentHandlerTest.cpp unittests/Tooling/CommentHandlerTest.cpp
index 117dfc3..14952e8 100644
--- unittests/Tooling/CommentHandlerTest.cpp
+++ unittests/Tooling/CommentHandlerTest.cpp
@@ -56,8 +56,8 @@ public:
CommentVerifier GetVerifier();
protected:
- virtual ASTFrontendAction* CreateTestAction() {
- return new CommentHandlerAction(this);
+ virtual std::unique_ptr<ASTFrontendAction> CreateTestAction() {
+ return llvm::make_unique<CommentHandlerAction>(this);
}
private:
diff --git unittests/Tooling/RefactoringCallbacksTest.cpp unittests/Tooling/RefactoringCallbacksTest.cpp
index c2b331c..230721c 100644
--- unittests/Tooling/RefactoringCallbacksTest.cpp
+++ unittests/Tooling/RefactoringCallbacksTest.cpp
@@ -25,9 +25,8 @@ void expectRewritten(const std::string &Code,
RefactoringCallback &Callback) {
MatchFinder Finder;
Finder.addMatcher(AMatcher, &Callback);
- std::unique_ptr<tooling::FrontendActionFactory> Factory(
- tooling::newFrontendActionFactory(&Finder));
- ASSERT_TRUE(tooling::runToolOnCode(Factory->create(), Code))
+ auto Factory = tooling::newFrontendActionFactory(&Finder);
+ ASSERT_TRUE(tooling::runToolOnCode(Factory.create(), Code))
<< "Parsing error in \"" << Code << "\"";
RewriterTestContext Context;
FileID ID = Context.createInMemoryFile("input.cc", Code);
diff --git unittests/Tooling/RefactoringTest.cpp unittests/Tooling/RefactoringTest.cpp
index ddb974a..d498ff8 100644
--- unittests/Tooling/RefactoringTest.cpp
+++ unittests/Tooling/RefactoringTest.cpp
@@ -276,7 +276,7 @@ template <typename T>
class TestVisitor : public clang::RecursiveASTVisitor<T> {
public:
bool runOver(StringRef Code) {
- return runToolOnCode(new TestAction(this), Code);
+ return runToolOnCode(llvm::make_unique<TestAction>(this), Code);
}
protected:
diff --git unittests/Tooling/TestVisitor.h unittests/Tooling/TestVisitor.h
index 2e64032..6cc95b0 100644
--- unittests/Tooling/TestVisitor.h
+++ unittests/Tooling/TestVisitor.h
@@ -62,8 +62,8 @@ public:
}
protected:
- virtual ASTFrontendAction* CreateTestAction() {
- return new TestAction(this);
+ virtual std::unique_ptr<ASTFrontendAction> CreateTestAction() {
+ return llvm::make_unique<TestAction>(this);
}
class FindConsumer : public ASTConsumer {
diff --git unittests/Tooling/ToolingTest.cpp unittests/Tooling/ToolingTest.cpp
index 2b57c16..37ebc3e 100644
--- unittests/Tooling/ToolingTest.cpp
+++ unittests/Tooling/ToolingTest.cpp
@@ -59,8 +59,10 @@ class FindTopLevelDeclConsumer : public clang::ASTConsumer {
TEST(runToolOnCode, FindsNoTopLevelDeclOnEmptyCode) {
bool FoundTopLevelDecl = false;
- EXPECT_TRUE(runToolOnCode(
- new TestAction(new FindTopLevelDeclConsumer(&FoundTopLevelDecl)), ""));
+ EXPECT_TRUE(
+ runToolOnCode(llvm::make_unique<TestAction>(
+ new FindTopLevelDeclConsumer(&FoundTopLevelDecl)),
+ ""));
EXPECT_FALSE(FoundTopLevelDecl);
}
@@ -97,13 +99,15 @@ bool FindClassDeclX(ASTUnit *AST) {
TEST(runToolOnCode, FindsClassDecl) {
bool FoundClassDeclX = false;
- EXPECT_TRUE(runToolOnCode(new TestAction(
- new FindClassDeclXConsumer(&FoundClassDeclX)), "class X;"));
+ EXPECT_TRUE(runToolOnCode(llvm::make_unique<TestAction>(
+ new FindClassDeclXConsumer(&FoundClassDeclX)),
+ "class X;"));
EXPECT_TRUE(FoundClassDeclX);
FoundClassDeclX = false;
- EXPECT_TRUE(runToolOnCode(new TestAction(
- new FindClassDeclXConsumer(&FoundClassDeclX)), "class Y;"));
+ EXPECT_TRUE(runToolOnCode(llvm::make_unique<TestAction>(
+ new FindClassDeclXConsumer(&FoundClassDeclX)),
+ "class Y;"));
EXPECT_FALSE(FoundClassDeclX);
}
@@ -118,9 +122,8 @@ TEST(buildASTFromCode, FindsClassDecl) {
}
TEST(newFrontendActionFactory, CreatesFrontendActionFactoryFromType) {
- std::unique_ptr<FrontendActionFactory> Factory(
- newFrontendActionFactory<SyntaxOnlyAction>());
- std::unique_ptr<FrontendAction> Action(Factory->create());
+ auto Factory = newFrontendActionFactory<SyntaxOnlyAction>();
+ std::unique_ptr<FrontendAction> Action = Factory.create();
EXPECT_TRUE(Action.get() != nullptr);
}
@@ -132,9 +135,8 @@ struct IndependentFrontendActionCreator {
TEST(newFrontendActionFactory, CreatesFrontendActionFactoryFromFactoryType) {
IndependentFrontendActionCreator Creator;
- std::unique_ptr<FrontendActionFactory> Factory(
- newFrontendActionFactory(&Creator));
- std::unique_ptr<FrontendAction> Action(Factory->create());
+ auto Factory = newFrontendActionFactory(&Creator);
+ std::unique_ptr<FrontendAction> Action = Factory.create();
EXPECT_TRUE(Action.get() != nullptr);
}
@@ -146,8 +148,8 @@ TEST(ToolInvocation, TestMapVirtualFile) {
Args.push_back("-Idef");
Args.push_back("-fsyntax-only");
Args.push_back("test.cpp");
- clang::tooling::ToolInvocation Invocation(Args, new SyntaxOnlyAction,
- Files.getPtr());
+ clang::tooling::ToolInvocation Invocation(
+ Args, llvm::make_unique<SyntaxOnlyAction>(), Files.getPtr());
Invocation.mapVirtualFile("test.cpp", "#include <abc>\n");
Invocation.mapVirtualFile("def/abc", "\n");
EXPECT_TRUE(Invocation.run());
@@ -165,8 +167,8 @@ TEST(ToolInvocation, TestVirtualModulesCompilation) {
Args.push_back("-Idef");
Args.push_back("-fsyntax-only");
Args.push_back("test.cpp");
- clang::tooling::ToolInvocation Invocation(Args, new SyntaxOnlyAction,
- Files.getPtr());
+ clang::tooling::ToolInvocation Invocation(
+ Args, llvm::make_unique<SyntaxOnlyAction>(), Files.getPtr());
Invocation.mapVirtualFile("test.cpp", "#include <abc>\n");
Invocation.mapVirtualFile("def/abc", "\n");
// Add a module.map file in the include directory of our header, so we trigger
@@ -206,9 +208,8 @@ TEST(newFrontendActionFactory, InjectsSourceFileCallbacks) {
Tool.mapVirtualFile("/a.cc", "void a() {}");
Tool.mapVirtualFile("/b.cc", "void b() {}");
- std::unique_ptr<FrontendActionFactory> Action(
- newFrontendActionFactory(&EndCallback, &EndCallback));
- Tool.run(Action.get());
+ auto Action = newFrontendActionFactory(&EndCallback, &EndCallback);
+ Tool.run(Action);
EXPECT_TRUE(EndCallback.Matched);
EXPECT_EQ(2u, EndCallback.BeginCalled);
@@ -233,9 +234,9 @@ struct SkipBodyAction : public clang::ASTFrontendAction {
};
TEST(runToolOnCode, TestSkipFunctionBody) {
- EXPECT_TRUE(runToolOnCode(new SkipBodyAction,
+ EXPECT_TRUE(runToolOnCode(llvm::make_unique<SkipBodyAction>(),
"int skipMe() { an_error_here }"));
- EXPECT_FALSE(runToolOnCode(new SkipBodyAction,
+ EXPECT_FALSE(runToolOnCode(llvm::make_unique<SkipBodyAction>(),
"int skipMeNot() { an_error_here }"));
}
@@ -249,7 +250,8 @@ TEST(runToolOnCodeWithArgs, TestNoDepFile) {
Args.push_back(DepFilePath.str());
Args.push_back("-MF");
Args.push_back(DepFilePath.str());
- EXPECT_TRUE(runToolOnCodeWithArgs(new SkipBodyAction, "", Args));
+ EXPECT_TRUE(
+ runToolOnCodeWithArgs(llvm::make_unique<SkipBodyAction>(), "", Args));
EXPECT_FALSE(llvm::sys::fs::exists(DepFilePath.str()));
EXPECT_FALSE(llvm::sys::fs::remove(DepFilePath.str()));
}
@@ -279,13 +281,12 @@ TEST(ClangToolTest, ArgumentAdjusters) {
ClangTool Tool(Compilations, std::vector<std::string>(1, "/a.cc"));
Tool.mapVirtualFile("/a.cc", "void a() {}");
- std::unique_ptr<FrontendActionFactory> Action(
- newFrontendActionFactory<SyntaxOnlyAction>());
+ auto Action = newFrontendActionFactory<SyntaxOnlyAction>();
bool Found = false;
bool Ran = false;
Tool.appendArgumentsAdjuster(new CheckSyntaxOnlyAdjuster(Found, Ran));
- Tool.run(Action.get());
+ Tool.run(Action);
EXPECT_TRUE(Ran);
EXPECT_TRUE(Found);
@@ -293,7 +294,7 @@ TEST(ClangToolTest, ArgumentAdjusters) {
Tool.clearArgumentsAdjusters();
Tool.appendArgumentsAdjuster(new CheckSyntaxOnlyAdjuster(Found, Ran));
Tool.appendArgumentsAdjuster(new ClangSyntaxOnlyAdjuster());
- Tool.run(Action.get());
+ Tool.run(Action);
EXPECT_TRUE(Ran);
EXPECT_FALSE(Found);
}
@@ -330,9 +331,8 @@ TEST(ClangToolTest, InjectDiagnosticConsumer) {
Tool.mapVirtualFile("/a.cc", "int x = undeclared;");
TestDiagnosticConsumer Consumer;
Tool.setDiagnosticConsumer(&Consumer);
- std::unique_ptr<FrontendActionFactory> Action(
- newFrontendActionFactory<SyntaxOnlyAction>());
- Tool.run(Action.get());
+ auto Action = newFrontendActionFactory<SyntaxOnlyAction>();
+ Tool.run(Action);
EXPECT_EQ(1u, Consumer.NumDiagnosticsSeen);
}
-------------- next part --------------
diff --git clang-modernize/Core/Transform.cpp clang-modernize/Core/Transform.cpp
index 8b51395..a004c9f 100644
--- clang-modernize/Core/Transform.cpp
+++ clang-modernize/Core/Transform.cpp
@@ -14,7 +14,6 @@
//===----------------------------------------------------------------------===//
#include "Core/Transform.h"
-#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/Basic/LangOptions.h"
#include "clang/Basic/SourceManager.h"
#include "clang/Frontend/CompilerInstance.h"
@@ -30,48 +29,6 @@ namespace {
using namespace tooling;
using namespace ast_matchers;
-/// \brief Custom FrontendActionFactory to produce FrontendActions that simply
-/// forward (Begin|End)SourceFileAction calls to a given Transform.
-class ActionFactory : public clang::tooling::FrontendActionFactory {
-public:
- ActionFactory(MatchFinder &Finder, Transform &Owner)
- : Finder(Finder), Owner(Owner) {}
-
- virtual FrontendAction *create() override {
- return new FactoryAdaptor(Finder, Owner);
- }
-
-private:
- class FactoryAdaptor : public ASTFrontendAction {
- public:
- FactoryAdaptor(MatchFinder &Finder, Transform &Owner)
- : Finder(Finder), Owner(Owner) {}
-
- ASTConsumer *CreateASTConsumer(CompilerInstance &, StringRef) {
- return Finder.newASTConsumer();
- }
-
- virtual bool BeginSourceFileAction(CompilerInstance &CI,
- StringRef Filename) override {
- if (!ASTFrontendAction::BeginSourceFileAction(CI, Filename))
- return false;
-
- return Owner.handleBeginSource(CI, Filename);
- }
-
- virtual void EndSourceFileAction() override {
- Owner.handleEndSource();
- return ASTFrontendAction::EndSourceFileAction();
- }
-
- private:
- MatchFinder &Finder;
- Transform &Owner;
- };
-
- MatchFinder &Finder;
- Transform &Owner;
-};
} // namespace
Transform::Transform(llvm::StringRef Name, const TransformOptions &Options)
@@ -126,8 +83,8 @@ Transform::addReplacementForCurrentTU(const clang::tooling::Replacement &R) {
return true;
}
-FrontendActionFactory *Transform::createActionFactory(MatchFinder &Finder) {
- return new ActionFactory(Finder, /*Owner=*/ *this);
+ActionFactory Transform::createActionFactory(MatchFinder &Finder) {
+ return ActionFactory(Finder, /*Owner=*/ *this);
}
Version Version::getFromString(llvm::StringRef VersionStr) {
diff --git clang-modernize/Core/Transform.h clang-modernize/Core/Transform.h
index 0cf94c9..b509fc8 100644
--- clang-modernize/Core/Transform.h
+++ clang-modernize/Core/Transform.h
@@ -18,6 +18,8 @@
#include "Core/IncludeExcludeInfo.h"
#include "Core/Refactoring.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Basic/LLVM.h"
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/Registry.h"
#include "llvm/Support/Timer.h"
@@ -70,6 +72,9 @@ struct TransformOptions {
RiskLevel MaxRiskLevel;
};
+
+class ActionFactory;
+
/// \brief Abstract base class for all C++11 migration transforms.
///
/// Subclasses must call createActionFactory() to create a
@@ -208,8 +213,7 @@ protected:
///
/// The factory returned by this function is responsible for calling back to
/// Transform to call handleBeginSource() and handleEndSource().
- clang::tooling::FrontendActionFactory *
- createActionFactory(clang::ast_matchers::MatchFinder &Finder);
+ ActionFactory createActionFactory(clang::ast_matchers::MatchFinder &Finder);
private:
const std::string Name;
@@ -322,4 +326,48 @@ protected:
typedef llvm::Registry<TransformFactory> TransformFactoryRegistry;
+/// \brief Custom FrontendActionFactory to produce FrontendActions that simply
+/// forward (Begin|End)SourceFileAction calls to a given Transform.
+class ActionFactory : public clang::tooling::FrontendActionFactory {
+public:
+ ActionFactory(clang::ast_matchers::MatchFinder &Finder, Transform &Owner)
+ : Finder(Finder), Owner(Owner) {}
+
+ virtual std::unique_ptr<clang::FrontendAction> create() const override {
+ return llvm::make_unique<FactoryAdaptor>(Finder, Owner);
+ }
+
+private:
+ class FactoryAdaptor : public clang::ASTFrontendAction {
+ public:
+ FactoryAdaptor(clang::ast_matchers::MatchFinder &Finder, Transform &Owner)
+ : Finder(Finder), Owner(Owner) {}
+
+ clang::ASTConsumer *CreateASTConsumer(clang::CompilerInstance &,
+ llvm::StringRef) {
+ return Finder.newASTConsumer();
+ }
+
+ virtual bool BeginSourceFileAction(clang::CompilerInstance &CI,
+ llvm::StringRef Filename) override {
+ if (!clang::ASTFrontendAction::BeginSourceFileAction(CI, Filename))
+ return false;
+
+ return Owner.handleBeginSource(CI, Filename);
+ }
+
+ virtual void EndSourceFileAction() override {
+ Owner.handleEndSource();
+ return clang::ASTFrontendAction::EndSourceFileAction();
+ }
+
+ private:
+ clang::ast_matchers::MatchFinder &Finder;
+ Transform &Owner;
+ };
+
+ clang::ast_matchers::MatchFinder &Finder;
+ Transform &Owner;
+};
+
#endif // CLANG_MODERNIZE_TRANSFORM_H
diff --git clang-modernize/LoopConvert/LoopConvert.cpp clang-modernize/LoopConvert/LoopConvert.cpp
index 39ece5b..3add0a7 100644
--- clang-modernize/LoopConvert/LoopConvert.cpp
+++ clang-modernize/LoopConvert/LoopConvert.cpp
@@ -48,7 +48,7 @@ int LoopConvertTransform::apply(const CompilationDatabase &Database,
LFK_PseudoArray, /*Owner=*/ *this);
Finder.addMatcher(makePseudoArrayLoopMatcher(), &PseudoarrrayLoopFixer);
- if (int result = LoopTool.run(createActionFactory(Finder))) {
+ if (int result = LoopTool.run(ActionFactory(Finder, *this))) {
llvm::errs() << "Error encountered during translation.\n";
return result;
}
diff --git clang-modernize/PassByValue/PassByValue.cpp clang-modernize/PassByValue/PassByValue.cpp
index 4e44a34..576c25a 100644
--- clang-modernize/PassByValue/PassByValue.cpp
+++ clang-modernize/PassByValue/PassByValue.cpp
@@ -35,7 +35,7 @@ int PassByValueTransform::apply(const tooling::CompilationDatabase &Database,
// make the replacer available to handleBeginSource()
this->Replacer = &Replacer;
- if (Tool.run(createActionFactory(Finder))) {
+ if (Tool.run(ActionFactory(Finder, *this))) {
llvm::errs() << "Error encountered during translation.\n";
return 1;
}
diff --git clang-modernize/ReplaceAutoPtr/ReplaceAutoPtr.cpp clang-modernize/ReplaceAutoPtr/ReplaceAutoPtr.cpp
index f08e9cd..e79901a 100644
--- clang-modernize/ReplaceAutoPtr/ReplaceAutoPtr.cpp
+++ clang-modernize/ReplaceAutoPtr/ReplaceAutoPtr.cpp
@@ -34,7 +34,7 @@ ReplaceAutoPtrTransform::apply(const CompilationDatabase &Database,
Finder.addMatcher(makeAutoPtrUsingDeclMatcher(), &Replacer);
Finder.addMatcher(makeTransferOwnershipExprMatcher(), &Fixer);
- if (Tool.run(createActionFactory(Finder))) {
+ if (Tool.run(ActionFactory(Finder, *this))) {
llvm::errs() << "Error encountered during translation.\n";
return 1;
}
diff --git clang-modernize/UseAuto/UseAuto.cpp clang-modernize/UseAuto/UseAuto.cpp
index 72b97ac..806d8a6 100644
--- clang-modernize/UseAuto/UseAuto.cpp
+++ clang-modernize/UseAuto/UseAuto.cpp
@@ -36,7 +36,7 @@ int UseAutoTransform::apply(const clang::tooling::CompilationDatabase &Database,
Finder.addMatcher(makeIteratorDeclMatcher(), &ReplaceIterators);
Finder.addMatcher(makeDeclWithNewMatcher(), &ReplaceNew);
- if (int Result = UseAutoTool.run(createActionFactory(Finder))) {
+ if (int Result = UseAutoTool.run(ActionFactory(Finder, *this))) {
llvm::errs() << "Error encountered during translation.\n";
return Result;
}
diff --git clang-modernize/UseNullptr/UseNullptr.cpp clang-modernize/UseNullptr/UseNullptr.cpp
index 9d63f76..00991bc 100644
--- clang-modernize/UseNullptr/UseNullptr.cpp
+++ clang-modernize/UseNullptr/UseNullptr.cpp
@@ -47,7 +47,7 @@ int UseNullptrTransform::apply(const CompilationDatabase &Database,
Finder.addMatcher(makeCastSequenceMatcher(), &Fixer);
- if (int result = UseNullptrTool.run(createActionFactory(Finder))) {
+ if (int result = UseNullptrTool.run(ActionFactory(Finder, *this))) {
llvm::errs() << "Error encountered during translation.\n";
return result;
}
diff --git clang-modernize/tool/ClangModernize.cpp clang-modernize/tool/ClangModernize.cpp
index 31e8925..9b25ce9 100644
--- clang-modernize/tool/ClangModernize.cpp
+++ clang-modernize/tool/ClangModernize.cpp
@@ -469,7 +469,7 @@ int main(int argc, const char **argv) {
if (FinalSyntaxCheck) {
ClangTool SyntaxTool(*Compilations, SourcePaths);
- if (SyntaxTool.run(newFrontendActionFactory<SyntaxOnlyAction>().get()) != 0)
+ if (SyntaxTool.run(newFrontendActionFactory<SyntaxOnlyAction>()) != 0)
return 1;
}
diff --git clang-tidy/ClangTidy.cpp clang-tidy/ClangTidy.cpp
index 7b903df..d95d8d3 100644
--- clang-tidy/ClangTidy.cpp
+++ clang-tidy/ClangTidy.cpp
@@ -324,7 +324,9 @@ ClangTidyStats runClangTidy(ClangTidyOptionsProvider *OptionsProvider,
public:
ActionFactory(ClangTidyASTConsumerFactory *ConsumerFactory)
: ConsumerFactory(ConsumerFactory) {}
- FrontendAction *create() override { return new Action(ConsumerFactory); }
+ std::unique_ptr<FrontendAction> create() const override {
+ return llvm::make_unique<Action>(ConsumerFactory);
+ }
private:
class Action : public ASTFrontendAction {
@@ -342,7 +344,7 @@ ClangTidyStats runClangTidy(ClangTidyOptionsProvider *OptionsProvider,
ClangTidyASTConsumerFactory *ConsumerFactory;
};
- Tool.run(new ActionFactory(new ClangTidyASTConsumerFactory(Context)));
+ Tool.run(ActionFactory(new ClangTidyASTConsumerFactory(Context)));
*Errors = Context.getErrors();
return Context.getStats();
}
diff --git modularize/Modularize.cpp modularize/Modularize.cpp
index e4841cc..cfb1ab3 100644
--- modularize/Modularize.cpp
+++ modularize/Modularize.cpp
@@ -672,8 +672,9 @@ public:
: Entities(Entities), PPTracker(preprocessorTracker),
HadErrors(HadErrors) {}
- virtual CollectEntitiesAction *create() {
- return new CollectEntitiesAction(Entities, PPTracker, HadErrors);
+ virtual std::unique_ptr<FrontendAction> create() const {
+ return llvm::make_unique<CollectEntitiesAction>(Entities, PPTracker,
+ HadErrors);
}
private:
@@ -729,7 +730,8 @@ int main(int Argc, const char **Argv) {
new FixedCompilationDatabase(Twine(PathBuf), CC1Arguments));
// Create preprocessor tracker, to watch for macro and conditional problems.
- std::unique_ptr<PreprocessorTracker> PPTracker(PreprocessorTracker::create());
+ std::unique_ptr<PreprocessorTracker> PPTracker =
+ PreprocessorTracker::create();
// Parse all of the headers, detecting duplicates.
EntityMap Entities;
@@ -737,7 +739,7 @@ int main(int Argc, const char **Argv) {
Tool.appendArgumentsAdjuster(new AddDependenciesAdjuster(Dependencies));
int HadErrors = 0;
HadErrors |= Tool.run(
- new ModularizeFrontendActionFactory(Entities, *PPTracker, HadErrors));
+ ModularizeFrontendActionFactory(Entities, *PPTracker, HadErrors));
// Create a place to save duplicate entity locations, separate bins per kind.
typedef SmallVector<Location, 8> LocationArray;
@@ -770,7 +772,8 @@ int main(int Argc, const char **Argv) {
for (EntryBinArray::iterator DI = EntryBins.begin(), DE = EntryBins.end();
DI != DE; ++DI, ++KindIndex) {
int ECount = DI->size();
- // If only 1 occurrence of this entity, skip it, as we only report duplicates.
+ // If only 1 occurrence of this entity, skip it, as we only report
+ // duplicates.
if (ECount <= 1)
continue;
LocationArray::iterator FI = DI->begin();
diff --git modularize/PreprocessorTracker.cpp modularize/PreprocessorTracker.cpp
index 0b99f21..ca5896e 100644
--- modularize/PreprocessorTracker.cpp
+++ modularize/PreprocessorTracker.cpp
@@ -248,6 +248,7 @@
#include "PreprocessorTracker.h"
#include "clang/Lex/MacroArgs.h"
#include "clang/Lex/PPCallbacks.h"
+#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/SmallSet.h"
#include "llvm/Support/StringPool.h"
#include "llvm/Support/raw_ostream.h"
@@ -1313,8 +1314,8 @@ private:
PreprocessorTracker::~PreprocessorTracker() {}
// Create instance of PreprocessorTracker.
-PreprocessorTracker *PreprocessorTracker::create() {
- return new PreprocessorTrackerImpl();
+std::unique_ptr<PreprocessorTracker> PreprocessorTracker::create() {
+ return llvm::make_unique<PreprocessorTrackerImpl>();
}
// Preprocessor callbacks for modularize.
diff --git modularize/PreprocessorTracker.h modularize/PreprocessorTracker.h
index 4f72d01..4097755 100644
--- modularize/PreprocessorTracker.h
+++ modularize/PreprocessorTracker.h
@@ -77,7 +77,7 @@ public:
virtual bool reportInconsistentConditionals(llvm::raw_ostream &OS) = 0;
// Create instance of PreprocessorTracker.
- static PreprocessorTracker *create();
+ static std::unique_ptr<PreprocessorTracker> create();
};
} // end namespace Modularize
diff --git module-map-checker/ModuleMapChecker.cpp module-map-checker/ModuleMapChecker.cpp
index 23d75dc..f0b8922 100644
--- module-map-checker/ModuleMapChecker.cpp
+++ module-map-checker/ModuleMapChecker.cpp
@@ -192,8 +192,8 @@ public:
ModuleMapCheckerFrontendActionFactory(ModuleMapChecker &Checker)
: Checker(Checker) {}
- virtual ModuleMapCheckerAction *create() {
- return new ModuleMapCheckerAction(Checker);
+ virtual std::unique_ptr<FrontendAction> create() const {
+ return llvm::make_unique<ModuleMapCheckerAction>(Checker);
}
private:
@@ -403,7 +403,7 @@ ModuleMapChecker::collectUmbrellaHeaderHeaders(StringRef UmbrellaHeaderName) {
// Create the tool and run the compilation.
ClangTool Tool(*Compilations, HeaderPath);
- int HadErrors = Tool.run(new ModuleMapCheckerFrontendActionFactory(*this));
+ int HadErrors = Tool.run(ModuleMapCheckerFrontendActionFactory(*this));
// If we had errors, exit early.
return HadErrors ? false : true;
diff --git pp-trace/PPTrace.cpp pp-trace/PPTrace.cpp
index ed955fb..6fbb8f5 100644
--- pp-trace/PPTrace.cpp
+++ pp-trace/PPTrace.cpp
@@ -136,8 +136,8 @@ public:
std::vector<CallbackCall> &CallbackCalls)
: Ignore(Ignore), CallbackCalls(CallbackCalls) {}
- virtual PPTraceAction *create() {
- return new PPTraceAction(Ignore, CallbackCalls);
+ virtual std::unique_ptr<FrontendAction> create() const {
+ return llvm::make_unique<PPTraceAction>(Ignore, CallbackCalls);
}
private:
@@ -199,8 +199,7 @@ int main(int Argc, const char **Argv) {
// Create the tool and run the compilation.
ClangTool Tool(*Compilations, SourcePaths);
- int HadErrors =
- Tool.run(new PPTraceFrontendActionFactory(Ignore, CallbackCalls));
+ int HadErrors = Tool.run(PPTraceFrontendActionFactory(Ignore, CallbackCalls));
// If we had errors, exit early.
if (HadErrors)
diff --git remove-cstr-calls/RemoveCStrCalls.cpp remove-cstr-calls/RemoveCStrCalls.cpp
index 3c95ad0..63c0615 100644
--- remove-cstr-calls/RemoveCStrCalls.cpp
+++ remove-cstr-calls/RemoveCStrCalls.cpp
@@ -233,5 +233,5 @@ int main(int argc, const char **argv) {
callee(methodDecl(hasName(StringCStrMethod))),
on(id("arg", expr())))))),
&Callback);
- return Tool.runAndSave(newFrontendActionFactory(&Finder).get());
+ return Tool.runAndSave(newFrontendActionFactory(&Finder));
}
diff --git tool-template/ToolTemplate.cpp tool-template/ToolTemplate.cpp
index 48a44ac..7be964f 100644
--- tool-template/ToolTemplate.cpp
+++ tool-template/ToolTemplate.cpp
@@ -103,5 +103,5 @@ int main(int argc, const char **argv) {
// Use Finder.addMatcher(...) to define the patterns in the AST that you
// want to match against. You are not limited to just one matcher!
- return Tool.run(newFrontendActionFactory(&Finder).get());
+ return Tool.run(newFrontendActionFactory(&Finder));
}
diff --git unittests/clang-modernize/IncludeDirectivesTest.cpp unittests/clang-modernize/IncludeDirectivesTest.cpp
index b135288..f217597 100644
--- unittests/clang-modernize/IncludeDirectivesTest.cpp
+++ unittests/clang-modernize/IncludeDirectivesTest.cpp
@@ -19,7 +19,8 @@ using namespace clang;
/// \brief A convenience method around \c tooling::runToolOnCodeWithArgs() that
/// adds the current directory to the include search paths.
-static void applyActionOnCode(FrontendAction *ToolAction, StringRef Code) {
+static void applyActionOnCode(std::unique_ptr<FrontendAction> ToolAction,
+ StringRef Code) {
SmallString<128> CurrentDir;
ASSERT_FALSE(llvm::sys::fs::current_path(CurrentDir));
@@ -33,8 +34,8 @@ static void applyActionOnCode(FrontendAction *ToolAction, StringRef Code) {
SmallString<128> InputFile(CurrentDir);
sys::path::append(InputFile, "input.cc");
- ASSERT_TRUE(
- tooling::runToolOnCodeWithArgs(ToolAction, Code, Args, InputFile.str()));
+ ASSERT_TRUE(tooling::runToolOnCodeWithArgs(std::move(ToolAction), Code, Args,
+ InputFile.str()));
}
namespace {
@@ -115,7 +116,8 @@ private:
std::string addIncludeInCode(StringRef Include, StringRef Code) {
tooling::Replacements Replaces;
- applyActionOnCode(new TestAddIncludeAction(Include, Replaces), Code);
+ applyActionOnCode(llvm::make_unique<TestAddIncludeAction>(Include, Replaces),
+ Code);
if (::testing::Test::HasFailure())
return "<<unexpected error from applyActionOnCode()>>";
@@ -236,11 +238,12 @@ TEST(IncludeDirectivesTest, ignoreHeadersMeantForMultipleInclusion) {
}
namespace {
-TestAddIncludeAction *makeIndirectTestsAction(const char *HeaderToModify,
- tooling::Replacements &Replaces) {
+std::unique_ptr<TestAddIncludeAction>
+makeIndirectTestsAction(const char *HeaderToModify,
+ tooling::Replacements &Replaces) {
StringRef IncludeToAdd = "c.h";
- TestAddIncludeAction *TestAction =
- new TestAddIncludeAction(IncludeToAdd, Replaces, HeaderToModify);
+ auto TestAction = llvm::make_unique<TestAddIncludeAction>(
+ IncludeToAdd, Replaces, HeaderToModify);
TestAction->mapVirtualHeader("c.h", "#pragma once\n");
TestAction->mapVirtualHeader("a.h", "#pragma once\n"
"#include <c.h>\n");
@@ -261,16 +264,16 @@ TEST(IncludeDirectivesTest, indirectIncludes) {
// a.h already includes c.h
{
- FrontendAction *Action = makeIndirectTestsAction("a.h", Replaces);
- ASSERT_NO_FATAL_FAILURE(applyActionOnCode(Action, Code));
+ auto Action = makeIndirectTestsAction("a.h", Replaces);
+ ASSERT_NO_FATAL_FAILURE(applyActionOnCode(std::move(Action), Code));
EXPECT_EQ(unsigned(0), Replaces.size());
}
// c.h is included before b.h but b.h doesn't include c.h directly, so check
// that it will be inserted.
{
- FrontendAction *Action = makeIndirectTestsAction("b.h", Replaces);
- ASSERT_NO_FATAL_FAILURE(applyActionOnCode(Action, Code));
+ auto Action = makeIndirectTestsAction("b.h", Replaces);
+ ASSERT_NO_FATAL_FAILURE(applyActionOnCode(std::move(Action), Code));
EXPECT_EQ("#include <c.h>\n\n\n",
tooling::applyAllReplacements("\n", Replaces));
}
@@ -281,11 +284,11 @@ static std::string addIncludeInGuardedHeader(StringRef IncludeToAdd,
StringRef GuardedHeaderCode) {
const char *GuardedHeaderName = "guarded.h";
tooling::Replacements Replaces;
- TestAddIncludeAction *TestAction =
- new TestAddIncludeAction(IncludeToAdd, Replaces, GuardedHeaderName);
+ auto TestAction = llvm::make_unique<TestAddIncludeAction>(
+ IncludeToAdd, Replaces, GuardedHeaderName);
TestAction->mapVirtualHeader(GuardedHeaderName, GuardedHeaderCode);
- applyActionOnCode(TestAction, "#include <guarded.h>\n");
+ applyActionOnCode(std::move(TestAction), "#include <guarded.h>\n");
if (::testing::Test::HasFailure())
return "<<unexpected error from applyActionOnCode()>>";
diff --git unittests/clang-modernize/TransformTest.cpp unittests/clang-modernize/TransformTest.cpp
index ae05f5a..5bbe5a8 100644
--- unittests/clang-modernize/TransformTest.cpp
+++ unittests/clang-modernize/TransformTest.cpp
@@ -153,8 +153,7 @@ TEST(Transform, Timings) {
// handleEndSource() calls to it.
CallbackForwarder Callbacks(T);
- Tool.run(
- clang::tooling::newFrontendActionFactory(&Factory, &Callbacks).get());
+ Tool.run(clang::tooling::newFrontendActionFactory(&Factory, &Callbacks));
EXPECT_TRUE(Factory.Called);
Transform::TimingVec::const_iterator I = T.timing_begin();
@@ -272,7 +271,7 @@ TEST(Transform, isFileModifiable) {
DummyTransform T("dummy", Options);
MatchFinder Finder;
Finder.addMatcher(varDecl().bind("decl"), new ModifiableCallback(T));
- Tool.run(tooling::newFrontendActionFactory(&Finder).get());
+ Tool.run(tooling::newFrontendActionFactory(&Finder));
}
TEST(VersionTest, Interface) {
diff --git unittests/clang-tidy/ClangTidyTest.h unittests/clang-tidy/ClangTidyTest.h
index 6fb0318..4a42257 100644
--- unittests/clang-tidy/ClangTidyTest.h
+++ unittests/clang-tidy/ClangTidyTest.h
@@ -49,14 +49,13 @@ std::string runCheckOnCode(StringRef Code,
Check.setContext(&Context);
std::vector<std::string> ArgCXX11(1, "-std=c++11");
- if (!tooling::runToolOnCodeWithArgs(new TestPPAction(Check, &Context), Code,
- ArgCXX11))
+ if (!tooling::runToolOnCodeWithArgs(
+ llvm::make_unique<TestPPAction>(Check, &Context), Code, ArgCXX11))
return "";
ast_matchers::MatchFinder Finder;
Check.registerMatchers(&Finder);
- std::unique_ptr<tooling::FrontendActionFactory> Factory(
- tooling::newFrontendActionFactory(&Finder));
- if (!tooling::runToolOnCodeWithArgs(Factory->create(), Code, ArgCXX11))
+ auto Factory = tooling::newFrontendActionFactory(&Finder);
+ if (!tooling::runToolOnCodeWithArgs(Factory.create(), Code, ArgCXX11))
return "";
DiagConsumer.finish();
tooling::Replacements Fixes;
More information about the cfe-dev
mailing list