<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, May 6, 2014 at 6:38 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div>On Tue, May 6, 2014 at 12:42 AM, Manuel Klimek <<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>> wrote:<br>
> On Mon, May 5, 2014 at 10:54 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
>><br>
>> On Mon, May 5, 2014 at 8:42 AM, Manuel Klimek <<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>> wrote:<br>
>> > On Mon, May 5, 2014 at 5:14 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>><br>
>> > wrote:<br>
>> >><br>
>> >> On Mon, May 5, 2014 at 3:10 AM, Manuel Klimek <<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>><br>
>> >> wrote:<br>
>> >> > On Thu, May 1, 2014 at 10:21 PM, Richard Smith<br>
>> >> > <<a href="mailto:richard@metafoo.co.uk" target="_blank">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" target="_blank">thakis@chromium.org</a>><br>
>> >> >> wrote:<br>
>> >> >>><br>
>> >> >>> On Thu, May 1, 2014 at 1:17 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">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" target="_blank">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" target="_blank">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<br>
>> >> >>> >>> for<br>
>> >> >>> >>> users<br>
>> >> >>> >>> since is not the way it is written in the documentation, like<br>
>> >> >>> >>> 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<br>
>> >> >>> > to<br>
>> >> >>> > work now...<br>
>> >> >>><br>
>> >> >>> The result of newFrontendActionFactory() used to be leaked. Now<br>
>> >> >>> 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>
>> 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>
><br>
><br>
> Sure, but then we a templated constructor wouldn't be enough, we'd need a<br>
> templated class. If that's what you meant from the start, I misunderstood.<br>
<br>
</div></div>Sorry, yes, I'm not sure what I was trying to say above (I hadn't<br>
looked at the code in detail at that point - so I was probably being a<br>
bit vague/wrong). But, yes - as you can see in the patches, the types<br>
can just be used directly.<br>
<div><br>
>> 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?<br>
><br>
><br>
> Well, we can't return a class with virtual methods by value, can we? We'd<br>
> need to get rid of the factories if we want to not use pointers (and you're<br>
> doing that in both patches).<br>
<br>
</div>Not necessarily. The factories could return by value:<br>
<br>
struct base { ... };<br>
struct d1 : base { ... };<br>
struct d2 : base { ... };<br>
<br>
d1 factory() { return d1(); }<br>
d2 factory(int) { return d2(); }<br>
<br>
and usage could either be:<br>
<br>
d1 x = factory();<br>
d2 y = factory(3);<br>
<br>
or:<br>
<br>
base &x = factory();<br>
base &y = factory(3);<br></blockquote><div><br></div><div>Having the factory "leak" what concrete object it passes back defeats the purpose of the factory.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
But all except 1 caller I touched are just passing the result straight<br>
to Tool.run, so this change doesn't affect them. The one caller that<br>
did:<br></blockquote><div><br></div><div>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.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">unique_ptr<factory> p;<br>
if (x)<br>
p = newFactory();<br>
else if (y)<br>
p = newFactory(a);<br>
else<br>
p = newFactory(b);<br>
Tool.run(p);<br>
<br>
and the change was to roll the "Tool.run" call up into the call site,<br>
which wasn't overly burdensome.<br>
<div><br>
>> 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>
>><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>
>> Having to specify the parameter doesn't seem terribly burdensome.<br>
><br>
> I find having a unique_ptr return not a terrible problem, so I'd argue it's<br>
> trade-offs.<br>
<br>
</div>Sure (though as shown above, this particular issue doesn't have to be<br>
traded off - we can still use function templates as builders and get<br>
the convenience of template argument deduction if it's important)<br>
<div><br>
>> > Also, there is an overload<br>
>> > newFrontendActionFactory<FrontendActionType>(),<br>
>> > and I think it has value that those form similar patterns.<br>
>><br>
>> Which would be similarly owned directly in the caller and passed by<br>
>> 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>
><br>
><br>
> I would strongly vote against removing something because "it's not<br>
> polymorphically destroyed", where we expect users of the library to own<br>
> instances of it. It's very easy to introduce a place where it's<br>
> polymorphically destroyed.<br>
<br>
</div>The same would be true of any type - we don't put virtual dtors on all<br>
types in case someone tries to polymorphically destroy them. Clang has<br>
warnings (off by default, though, unfortunately<br>
-Wdelete-non-virtual-dtor) that can catch this bug if it's ever<br>
written.<br></blockquote><div><br></div><div>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.</div>
<div><br></div><div>That means, in a library that is exposed to users:</div><div>- a class that's meant to be inherited from by users should have a virtual dtor</div><div>- classes and methods shouldn't take ownership, unless they are basically glorified containers</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>
> Because of that I think having virtual functions<br>
> but not having a virtual destructor is an anti-pattern.<br>
<br>
</div>I don't agree - there are lots of types that are generally not<br>
polymorphically owned but are polymorphically used in idioms just like<br>
this one. Polymorphic usage doesn't imply polymorphic<br>
ownership/destruction.<br></blockquote><div><br></div><div>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).</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>
> My main problem with the attached patches is actually that it looks to me<br>
> like they change ownership in the runTool* functions (if I'm not missing<br>
> something).<br>
<br>
</div>Strangely enough, I didn't change the ownership of those functions -<br>
that API change just reflects the reality of the API. Notice existing<br>
callers (before my patch) passed "Factory.create()" (which returns a<br>
pointer the caller should take ownership of - most of the<br>
implementations are of the form "return new ...") and the<br></blockquote><div><br></div><div>Yes, that was an ownership bug in the existing use cases.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
implementation agrees in a somewhat circular way: runToolOnCode<br>
creates a ToolInvocation, which builds a SingleFrontendActionFactory<br>
which returns the FAction from its 'create()' function... which<br>
returns ownership.</blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
At least that's my understanding of the ownership model. (other<br>
evidence that create() returns ownership - see Tooling.cpp:259, taking<br>
the result of create() and initializing a unique_ptr with it)<br></blockquote><div><br></div><div>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, but I still am torn, because I value the idea that you can use the same Action for multiple runTool* calls.</div>
<div><br></div><div>The original question was about the FrontendActionFactory objects though - here I am still convinced that our interfaces should not take ownership.</div><div><br></div><div>Cheers,</div><div>/Manuel</div>
<div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span><font color="#888888"><br>
- David<br>
</font></span><div><div><br>
><br>
> Cheers,<br>
> /Manuel<br>
><br>
>><br>
>> - David<br>
>><br>
>> ><br>
>> >><br>
>> >><br>
>> >> > that we want to work<br>
>> >> > with pointers (because they have identity - we want the same one to<br>
>> >> > 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" target="_blank">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" target="_blank">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>
><br>
</div></div></blockquote></div><br></div></div>