[cfe-dev] Problem with a change on newFrontendActionFactory

Tue May 6 00:42:37 PDT 2014

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.

> 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).

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.

>
> > 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. Because of that I think having virtual functions
but not having a virtual destructor is an anti-pattern.

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).

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 --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20140506/38db1343/attachment.html>