<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, May 5, 2014 at 10:54 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class=""><div class="h5">On Mon, May 5, 2014 at 8:42 AM, Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>> wrote:<br>

> On Mon, May 5, 2014 at 5:14 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>><br>
>> On Mon, May 5, 2014 at 3:10 AM, Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>> wrote:<br>
>> > On Thu, May 1, 2014 at 10:21 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>><br>
>> > wrote:<br>
>> >><br>
>> >> On Thu, May 1, 2014 at 1:19 PM, Nico Weber <<a href="mailto:thakis@chromium.org">thakis@chromium.org</a>> wrote:<br>
>> >>><br>
>> >>> On Thu, May 1, 2014 at 1:17 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>><br>
>> >>> wrote:<br>
>> >>> > On Thu, May 1, 2014 at 1:12 PM, Richard Smith<br>
>> >>> > <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>><br>
>> >>> > wrote:<br>
>> >>> >> On Thu, May 1, 2014 at 12:55 PM, Etienne Ollivier<br>
>> >>> >> <<a href="mailto:eollivier@bsu.edu">eollivier@bsu.edu</a>><br>
>> >>> >> wrote:<br>
>> >>> >>><br>
>> >>> >>> Hello,<br>
>> >>> >>> I updated my clang repository recently and I an error appeared<br>
>> >>> >>> that<br>
>> >>> >>> was<br>
>> >>> >>> not<br>
>> >>> >>> there before:<br>
>> >>> >>><br>
>> >>> >>> error: no viable conversion from<br>
>> >>> >>> 'std::unique_ptr<FrontendActionFactory>'<br>
>> >>> >>> to<br>
>> >>> >>>       'clang::tooling::ToolAction *'<br>
>> >>> >>>         return<br>
>> >>> >>> Tool.run(newFrontendActionFactory<MyPluginASTAction>());<br>
>> >>> >>><br>
>> >>> >>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~<br>
>> >>> >>><br>
>> >>> >>> It is because newFrontendActionFactory has been changed to work<br>
>> >>> >>> with<br>
>> >>> >>> std::unique_ptr. So if I change my code to<br>
>> >>> >>>    return<br>
>> >>> >>> Tool.run(&(*newFrontendActionFactory<MyPluginASTAction>()));<br>
>> >>> >><br>
>> >>> >><br>
>> >>> >> You can use .get() rather than the slightly non-obvious &*.<br>
>> >>> >><br>
>> >>> >>><br>
>> >>> >>> it works. The only little problem is that it can be confusing for<br>
>> >>> >>> users<br>
>> >>> >>> since is not the way it is written in the documentation, like on<br>
>> >>> >>> this<br>
>> >>> >>> pages:<br>
>> >>> >>> <a href="http://clang.llvm.org/docs/LibTooling.html" target="_blank">http://clang.llvm.org/docs/LibTooling.html</a><br>
>> >>> >>> <a href="http://clang.llvm.org/docs/LibASTMatchersTutorial.html" target="_blank">http://clang.llvm.org/docs/LibASTMatchersTutorial.html</a><br>
>> >>> >><br>
>> >>> >><br>
>> >>> >> Thanks, I've updated the documentation.<br>
>> >>> ><br>
>> >>> > I'm trying to understand how the ownership used to work/is meant to<br>
>> >>> > work now...<br>
>> >>><br>
>> >>> The result of newFrontendActionFactory() used to be leaked. Now it's<br>
>> >>> freed at the end-of-statement cleanup of the returned (invisible)<br>
>> >>> unique_ptr temporary.<br>
>> >><br>
>> >><br>
>> >> Why do we need to heap-allocate the FrontendActionFactory at all?<br>
>> ><br>
>> ><br>
>> > Technically we don't. There's just some ways to create the<br>
>> > FrontendActionFactory via templated factory functions<br>
>><br>
>> The current factories don't seem to make dynamic choices (or even<br>
>> templated choices) about which type to return (I may've missed<br>
>> something, though) - and the internal templating could still be<br>
>> implemented via a ctor template instead, I think.<br>
><br>
><br>
> How would it store the pointer to the FactoryT* ConsumerFactory?<br>
<br>
</div></div>I'm not sure I understand - it just takes it as a ctor parameter and<br>
stores it, doesn't it? Same as when the factory function is used.<br></blockquote><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
You talk about needing these things to not move around - so you can<br>
point to them - but even that doesn't seem relevant to the<br>
construction phase. If these factories returned by value and then that<br>
value was passed by const-ref to Tool.run, what would break?</blockquote><div><br></div><div>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).</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> I don't<br>
think anything would - the constructor of the FrontendActiorFactories<br>
don't appear to leak pointers to themselves. So if you prefer the<br>
factory function syntax, you can keep that. The prototype patches<br>
attached do not, though (in case you're right about immovability).<br>
<div class=""><br>
> Sure, a<br>
> templated class would work (basically just instantiate the<br>
> FrontendActionFactoryAdapter), but the problem is that then you'd always<br>
> need to specify the template argument.<br>
<br>
</div>Having to specify the parameter doesn't seem terribly burdensome.<br></blockquote><div><br></div><div>I find having a unique_ptr return not a terrible problem, so I'd argue it's trade-offs.</div><div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class=""><br>
> Also, there is an overload newFrontendActionFactory<FrontendActionType>(),<br>
> and I think it has value that those form similar patterns.<br>
<br>
</div>Which would be similarly owned directly in the caller and passed by reference.<br>
<br>
Tool.run(SimpleFrontendActionFactory<clang::ento::AnalysisAction>());<br>
<br>
Various cleanup/modification was required, including making<br>
FrontendActionFactory's functions const (so the temporary could be<br>
passed by const ref) - all existing implementations except the weird<br>
SingleFrontendActionFactory, required no changes. (well, technically<br>
SingleFrontendActionFactory would've required no changes if I hadn't<br>
fixed its raw pointer ownership to be a unique_ptr ownership - then<br>
the unique_ptr had to be mutable)<br>
<br>
It should be possible to remove the virtual dtor from<br>
FrontendActionFactory hierarchy too, since it's never polymorphically<br>
destroyed, only polymorphically used.<br></blockquote><div><br></div><div>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.</div>
<div><br></div><div>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). </div><div><br></div><div>Cheers,</div>
<div>/Manuel</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span class=""><font color="#888888"><br>
- David<br>
</font></span><div class=""><div class="h5"><br>
><br>
>><br>
>><br>
>> > that we want to work<br>
>> > with pointers (because they have identity - we want the same one to be<br>
>> > passable to different runs and work with the data).<br>
>><br>
>> In terms of pointer identity - the idea would be to create a<br>
>> FrontendActionFactory as a direct (rather than unique_ptr) local<br>
>> variable and pass it to each run call - the object would never move<br>
>> around, so pointers to it would remain valid throughout its lifetime.<br>
>><br>
>> Or am I picturing something different from what you have in mind?<br>
>><br>
>> - David<br>
>><br>
>> ><br>
>> >><br>
>> >><br>
>> >> _______________________________________________<br>
>> >> cfe-dev mailing list<br>
>> >> <a href="mailto:cfe-dev@cs.uiuc.edu">cfe-dev@cs.uiuc.edu</a><br>
>> >> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev</a><br>
>> >><br>
>> ><br>
>> ><br>
>> > _______________________________________________<br>
>> > cfe-dev mailing list<br>
>> > <a href="mailto:cfe-dev@cs.uiuc.edu">cfe-dev@cs.uiuc.edu</a><br>
>> > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev</a><br>
>> ><br>
><br>
><br>
</div></div></blockquote></div><br></div></div>