[cfe-dev] Problem with a change on newFrontendActionFactory

[Manuel Klimek]

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

Also, there is an overload newFrontendActionFactory<FrontendActionType>(),
and I think it has value that those form similar patterns.


>
> > 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/20140505/43d0c32f/attachment.html>