[cfe-dev] Problem with a change on newFrontendActionFactory
David Blaikie
dblaikie at gmail.com
Tue May 6 09:38:47 PDT 2014
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);
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:
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.
> 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.
> 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
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)
- 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
>> >> >
>> >
>> >
>
>
More information about the cfe-dev
mailing list