<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Jun 26, 2014 at 1:25 AM, 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 class="HOEnZb"><div class="h5">On Tue, Jun 3, 2014 at 12:44 AM, Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>> wrote:<br>

> On Tue, Jun 3, 2014 at 12:16 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>><br>
>> On Mon, Jun 2, 2014 at 1:34 AM, Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>> wrote:<br>
>> > Sorry for the late answer. I had it unread in my inbox and was mulling<br>
>> > over<br>
>> > various aspects to it.<br>
>> > My general problem with the whole thing is that even if your solution is<br>
>> > pareto-better (which I personally don't think, but let's assume), I<br>
>> > don't<br>
>> > think it matters enough to be worth the effort of change.<br>
>> > So, I'm not opposed to interface changes, I'm opposed to interface<br>
>> > changes<br>
>> > that don't provide significant enough value to pay for the cost. I am<br>
>> > also<br>
>> > not sure which changes exactly we're now talking about - I think it<br>
>> > would<br>
>> > help if you could split this CL in to the parts that (I hope) are<br>
>> > non-controversial, so we have solved the actual problem, and the parts<br>
>> > that<br>
>> > are controversial.<br>
>> ><br>
>> > On Wed, May 7, 2014 at 6:07 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>><br>
>> > wrote:<br>
>> >><br>
>> >> On Wed, May 7, 2014 at 4:46 AM, Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>><br>
>> >> wrote:<br>
>> >> > On Tue, May 6, 2014 at 6:38 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>><br>
>> >> > wrote:<br>
>> >> >><br>
>> >> >> On Tue, May 6, 2014 at 12:42 AM, Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>><br>
>> >> >> wrote:<br>
>> >> >> > On Mon, May 5, 2014 at 10:54 PM, David Blaikie<br>
>> >> >> > <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>><br>
>> >> >> > wrote:<br>
>> >> >> >><br>
>> >> >> >> On Mon, May 5, 2014 at 8:42 AM, Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>><br>
>> >> >> >> wrote:<br>
>> >> >> >> > On Mon, May 5, 2014 at 5:14 PM, David Blaikie<br>
>> >> >> >> > <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>><br>
>> >> >> >> > wrote:<br>
>> >> >> >> >><br>
>> >> >> >> >> On Mon, May 5, 2014 at 3:10 AM, Manuel Klimek<br>
>> >> >> >> >> <<a href="mailto:klimek@google.com">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">richard@metafoo.co.uk</a>><br>
>> >> >> >> >> > wrote:<br>
>> >> >> >> >> >><br>
>> >> >> >> >> >> On Thu, May 1, 2014 at 1:19 PM, Nico Weber<br>
>> >> >> >> >> >> <<a href="mailto:thakis@chromium.org">thakis@chromium.org</a>><br>
>> >> >> >> >> >> wrote:<br>
>> >> >> >> >> >>><br>
>> >> >> >> >> >>> On Thu, May 1, 2014 at 1:17 PM, David Blaikie<br>
>> >> >> >> >> >>> <<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<br>
>> >> >> >> >> >>> >>> 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>
>> >> >> >> >> >>> >>><br>
>> >> >> >> >> >>> >>> Tool.run(newFrontendActionFactory<MyPluginASTAction>());<br>
>> >> >> >> >> >>> >>><br>
>> >> >> >> >> >>> >>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~<br>
>> >> >> >> >> >>> >>><br>
>> >> >> >> >> >>> >>> It is because newFrontendActionFactory has been<br>
>> >> >> >> >> >>> >>> changed<br>
>> >> >> >> >> >>> >>> to<br>
>> >> >> >> >> >>> >>> work<br>
>> >> >> >> >> >>> >>> with<br>
>> >> >> >> >> >>> >>> std::unique_ptr. So if I change my code to<br>
>> >> >> >> >> >>> >>>    return<br>
>> >> >> >> >> >>> >>><br>
>> >> >> >> >> >>> >>><br>
>> >> >> >> >> >>> >>> Tool.run(&(*newFrontendActionFactory<MyPluginASTAction>()));<br>
>> >> >> >> >> >>> >><br>
>> >> >> >> >> >>> >><br>
>> >> >> >> >> >>> >> You can use .get() rather than the slightly non-obvious<br>
>> >> >> >> >> >>> >> &*.<br>
>> >> >> >> >> >>> >><br>
>> >> >> >> >> >>> >>><br>
>> >> >> >> >> >>> >>> it works. The only little problem is that it can be<br>
>> >> >> >> >> >>> >>> confusing<br>
>> >> >> >> >> >>> >>> for<br>
>> >> >> >> >> >>> >>> users<br>
>> >> >> >> >> >>> >>> since is not the way it is written in the<br>
>> >> >> >> >> >>> >>> documentation,<br>
>> >> >> >> >> >>> >>> 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<br>
>> >> >> >> >> >>> > work/is<br>
>> >> >> >> >> >>> > meant<br>
>> >> >> >> >> >>> > to<br>
>> >> >> >> >> >>> > work now...<br>
>> >> >> >> >> >>><br>
>> >> >> >> >> >>> The result of newFrontendActionFactory() used to be<br>
>> >> >> >> >> >>> leaked.<br>
>> >> >> >> >> >>> Now<br>
>> >> >> >> >> >>> it's<br>
>> >> >> >> >> >>> freed at the end-of-statement cleanup of the returned<br>
>> >> >> >> >> >>> (invisible)<br>
>> >> >> >> >> >>> unique_ptr temporary.<br>
>> >> >> >> >> >><br>
>> >> >> >> >> >><br>
>> >> >> >> >> >> Why do we need to heap-allocate the FrontendActionFactory<br>
>> >> >> >> >> >> at<br>
>> >> >> >> >> >> 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<br>
>> >> >> >> >> even<br>
>> >> >> >> >> templated choices) about which type to return (I may've missed<br>
>> >> >> >> >> something, though) - and the internal templating could still<br>
>> >> >> >> >> be<br>
>> >> >> >> >> implemented via a ctor template instead, I think.<br>
>> >> >> >> ><br>
>> >> >> >> ><br>
>> >> >> >> > How would it store the pointer to the FactoryT*<br>
>> >> >> >> > ConsumerFactory?<br>
>> >> >> >><br>
>> >> >> >> I'm not sure I understand - it just takes it as a ctor parameter<br>
>> >> >> >> 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<br>
>> >> >> > need<br>
>> >> >> > a<br>
>> >> >> > templated class. If that's what you meant from the start, I<br>
>> >> >> > misunderstood.<br>
>> >> >><br>
>> >> >> 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<br>
>> >> >> a<br>
>> >> >> bit vague/wrong). But, yes - as you can see in the patches, the<br>
>> >> >> types<br>
>> >> >> can just be used directly.<br>
>> >> >><br>
>> >> >> >> You talk about needing these things to not move around - so you<br>
>> >> >> >> 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<br>
>> >> >> >> 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<br>
>> >> >> > we?<br>
>> >> >> > We'd<br>
>> >> >> > need to get rid of the factories if we want to not use pointers<br>
>> >> >> > (and<br>
>> >> >> > you're<br>
>> >> >> > doing that in both patches).<br>
>> >> >><br>
>> >> >> 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>
>> >> ><br>
>> >> ><br>
>> >> > Having the factory "leak" what concrete object it passes back defeats<br>
>> >> > the<br>
>> >> > purpose of the factory.<br>
>> >><br>
>> >> Not necessarily - you said one of the benefits was template argument<br>
>> >> deduction and I'm just demonstrating that that can be achieved without<br>
>> >> dynamic allocation.<br>
>> ><br>
>> ><br>
>> > I don't see how that argument gets us any further. My arguments are<br>
>> > always<br>
>> > based on the full context of a specific implementation, not specific<br>
>> > theoretical issues.<br>
>> ><br>
>> >><br>
>> >> >> But all except 1 caller I touched are just passing the result<br>
>> >> >> straight<br>
>> >> >> to Tool.run, so this change doesn't affect them. The one caller that<br>
>> >> >> did:<br>
>> >> ><br>
>> >> ><br>
>> >> > The problem with an interface intended for use outside of Clang's<br>
>> >> > tree<br>
>> >> > is<br>
>> >> > that I don't think arguing based on what we find in Clang's tree<br>
>> >> > (which<br>
>> >> > are<br>
>> >> > mostly tests) is good enough.<br>
>> >><br>
>> >> We have 4-5 tools, that seemed like a reasonable bunch of use-cases.<br>
>> >><br>
>> >> I'm happy to go and look inside Google (where I assume the vast<br>
>> >> majority of tools are so far) and see if we see anything else.<br>
>> >><br>
>> >> Are there other things you think would be appropriate to use to<br>
>> >> evaluate<br>
>> >> this?<br>
>> ><br>
>> ><br>
>> > Patch it into the internal code-base, make sure all reverse dependency's<br>
>> > tests pass.<br>
>> ><br>
>> >><br>
>> >><br>
>> >> >> 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<br>
>> >> >> site,<br>
>> >> >> which wasn't overly burdensome.<br>
>> >> >><br>
>> >> >> >> I don't<br>
>> >> >> >> think anything would - the constructor of the<br>
>> >> >> >> 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<br>
>> >> >> >> immovability).<br>
>> >> >> >><br>
>> >> >> >> > Sure, a<br>
>> >> >> >> > templated class would work (basically just instantiate the<br>
>> >> >> >> > FrontendActionFactoryAdapter), but the problem is that then<br>
>> >> >> >> > you'd<br>
>> >> >> >> > 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<br>
>> >> >> > argue<br>
>> >> >> > it's<br>
>> >> >> > trade-offs.<br>
>> >> >><br>
>> >> >> Sure (though as shown above, this particular issue doesn't have to<br>
>> >> >> 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>
>> >> >><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<br>
>> >> >> >> by<br>
>> >> >> >> reference.<br>
>> >> >> >><br>
>> >> >> >><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<br>
>> >> >> >> be<br>
>> >> >> >> passed by const ref) - all existing implementations except the<br>
>> >> >> >> weird<br>
>> >> >> >> SingleFrontendActionFactory, required no changes. (well,<br>
>> >> >> >> technically<br>
>> >> >> >> SingleFrontendActionFactory would've required no changes if I<br>
>> >> >> >> hadn't<br>
>> >> >> >> fixed its raw pointer ownership to be a unique_ptr ownership -<br>
>> >> >> >> 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<br>
>> >> >> >> 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<br>
>> >> >> > to<br>
>> >> >> > own<br>
>> >> >> > instances of it. It's very easy to introduce a place where it's<br>
>> >> >> > polymorphically destroyed.<br>
>> >> >><br>
>> >> >> The same would be true of any type - we don't put virtual dtors on<br>
>> >> >> all<br>
>> >> >> types in case someone tries to polymorphically destroy them. Clang<br>
>> >> >> 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>
>> >> ><br>
>> >> ><br>
>> >> > The problem is that if the class is to be used / extended by users of<br>
>> >> > a<br>
>> >> > library, you limit the users' choices when you have a class that is<br>
>> >> > intended<br>
>> >> > to be inherited from, but doesn't have a virtual destructor. If clang<br>
>> >> > was<br>
>> >> > only a program, and not a library, I'd basically agree with most<br>
>> >> > points<br>
>> >> > you<br>
>> >> > make. Since it is a library, I put a lot of emphasis on what<br>
>> >> > interface a<br>
>> >> > potential user gets, how easy it is to write bugs against that<br>
>> >> > interface<br>
>> >> > and<br>
>> >> > understand that interface, and how much flexibility the user gets to<br>
>> >> > design<br>
>> >> > the software the way they want to.<br>
>> >> ><br>
>> >> > That means, in a library that is exposed to users:<br>
>> >> > - a class that's meant to be inherited from by users should have a<br>
>> >> > virtual<br>
>> >> > dtor<br>
>> >><br>
>> >> While I realize the Tooling API is more API than most of Clang/LLVM<br>
>> >> (which is all intended to be a library and is used as such to varying<br>
>> >> degrees) this isn't necessarily the prevailing attitude. It's come up<br>
>> >> a few times that we've wanted to remove virtual dtors from<br>
>> >> non-polymorphicalyl owned types but currently warnings stop us from<br>
>> >> doing that (-Wvirtual-dtor is on, in part because Google has that on<br>
>> >> internally and we didn't have a way to disable that for LLVM without<br>
>> >> disabling it for all projects that use LLVM - we now do, so we can<br>
>> >> probably turn that warning off in favor of -Wdelete-non-virtual-dtor).<br>
>> >> This has been discussed a few times and seems to be a fairly<br>
>> >> unambiguous prevailing attitude.<br>
>> >><br>
>> >> <a href="http://comments.gmane.org/gmane.comp.compilers.llvm.cv" target="_blank">http://comments.gmane.org/gmane.comp.compilers.llvm.cv</a>, but now you're<br>
>> >> saying you only theoretically want to do that?s/184558<br>
>> >><br>
>> >><br>
>> >> <a href="http://comments.gmane.org/gmane.comp.compilers.llvm.cvs/184558" target="_blank">http://comments.gmane.org/gmane.comp.compilers.llvm.cvs/184558</a><br>
>> >><br>
>> >> Which reminds me, your concern about bugs can be alleviated, I think,<br>
>> >> by making the base dtor protected and the derived classes final.<br>
>> ><br>
>> ><br>
>> > Following the links I don't see arguments about specific cases where we<br>
>> > want<br>
>> > *external* users of an interface to keep polymorphical ownership of a<br>
>> > class.<br>
>> > Because that's my argument, and if you disagree, I'd like to hear an<br>
>> > argument on why you think this is not a useful feature to have for users<br>
>> > of<br>
>> > this specific interface.<br>
>><br>
>> Because it's not a feature of our library. To me, making the dtor<br>
>> virtual is the same as making any other function virtual. You do it<br>
>> because it's intended to be overriden by users for use cases you have,<br>
>> practically speaking, use cases of your library. Our library never<br>
>> owns these things so I wouldn't make the dtor virtual.<br>
><br>
><br>
> The problem is that other methods don't limit what users of the library can<br>
> do with their own objects,<br>
<br>
</div></div>It does to a degree if we put a non-virtual function in a base class<br>
(or simply don't include a function (even a pure virtual one) in a<br>
base class that a user would like) then the user can't use our<br>
interface to invoke those functions polymorphically. If the user wants<br>
to dynamically own their things, they can introduce an immediate<br>
derived class of FrontendActionFactory and give their derived class a<br>
virtual function - then they can own their own factories dynamically.<br>
They still can't own the default factories dynamically (just as they<br>
can't own vector/list/etc polymorphically).<br>
<br>
In any case, the first set of patches doesn't actually remove any<br>
virtual dtors, it just changes the factories, factories' factory<br>
functions, and common usage to not /require/ polymorphic ownership.<br>
I'll look at follow up patches to see about protecting and<br>
devirtualizing the dtor, etc.<br>
<div class=""><br>
> but the destructor does. We intend users to<br>
> implement an interface, so taking the possibility away from them to do<br>
> ownership the way they like it seems like a rather radical move.<br>
><br>
> I am with you for classes where we actually want to take ownership (let's<br>
> assume we'd have a method for users to inject their own AST nodes, but we<br>
> want to take ownership of the nodes they give us). In that case, I'd also<br>
> vote for making the destructor protected.<br>
<br>
</div>Hmm, not sure I follow the case your describing here. If we took<br>
ownership based on some base class, we'd need the base class dtor to<br>
be public (or befriended) and virtual.<br>
<div><div class="h5"><br>
>> >> > - classes and methods shouldn't take ownership, unless they are<br>
>> >> > basically<br>
>> >> > glorified containers<br>
>> >><br>
>> >> The ownership issues with runTool functions I'm not passing judgment<br>
>> >> on - it was merely a mechanical change to reflect the reality of the<br>
>> >> API as it stands today. If that reality can be changed/fixed, I'm OK<br>
>> >> with that.<br>
>> >><br>
>> >> >> > Because of that I think having virtual functions<br>
>> >> >> > but not having a virtual destructor is an anti-pattern.<br>
>> >> >><br>
>> >> >> I don't agree - there are lots of types that are generally not<br>
>> >> >> polymorphically owned but are polymorphically used in idioms just<br>
>> >> >> like<br>
>> >> >> this one. Polymorphic usage doesn't imply polymorphic<br>
>> >> >> ownership/destruction.<br>
>> >> ><br>
>> >> ><br>
>> >> > No, it doesn't - but we have style rules because with C++ without<br>
>> >> > rules<br>
>> >> > it<br>
>> >> > is so easy to shoot your foot off that even compiler engineers don't<br>
>> >> > use<br>
>> >> > it<br>
>> >> > that way... Having a  rule to always add a virtual dtor if a class<br>
>> >> > has<br>
>> >> > at<br>
>> >> > least one virtual function makes reasoning about code way easier, and<br>
>> >> > not<br>
>> >> > writing a bug is always the least costly variant (even when the<br>
>> >> > compiler<br>
>> >> > can<br>
>> >> > catch it).<br>
>> >><br>
>> >> >> > My main problem with the attached patches is actually that it<br>
>> >> >> > looks<br>
>> >> >> > to<br>
>> >> >> > me<br>
>> >> >> > like they change ownership in the runTool* functions (if I'm not<br>
>> >> >> > missing<br>
>> >> >> > something).<br>
>> >> >><br>
>> >> >> Strangely enough, I didn't change the ownership of those functions -<br>
>> >> >> that API change just reflects the reality of the API. Notice<br>
>> >> >> 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>
>> >> ><br>
>> >> ><br>
>> >> > Yes, that was an ownership bug in the existing use cases.<br>
>> >><br>
>> >> 'was'? Has it been fixed? (I don't see the fix)<br>
>> >><br>
>> >> And where's the bug, exactly? (I just reasoned about the ownership<br>
>> >> semantics of the code, I haven't tried to understand what the<br>
>> >> /intended/ semantics are)<br>
>> >><br>
>> >> I assume the Factory.create() functions are intended to return<br>
>> >> ownership (should return std::unique_ptr<>).<br>
>> ><br>
>> ><br>
>> > Yes.<br>
>> ><br>
>> >><br>
>> >> Or are you thinking that<br>
>> >> Factory.create() could return some other kind of RAII wrapper that<br>
>> >> actually returns ownership to the Factory so that a singleton factory<br>
>> >> could be used more than once as long as each use was non-overlapping?<br>
>> >><br>
>> >> >> 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.<br>
>> >> >><br>
>> >> >><br>
>> >> >> At least that's my understanding of the ownership model. (other<br>
>> >> >> evidence that create() returns ownership - see Tooling.cpp:259,<br>
>> >> >> taking<br>
>> >> >> the result of create() and initializing a unique_ptr with it)<br>
>> >> ><br>
>> >> ><br>
>> >> > Sure, but that means create() returns ownership, not that runTool*<br>
>> >> > should<br>
>> >> > take it. I can see the reasons why we would want them to take<br>
>> >> > ownership,<br>
>> >><br>
>> >> If create returns ownership, how can runTool* not take ownership?<br>
>> ><br>
>> ><br>
>> > std::unique_ptr<FrontendAction> Action(Factory->Create());<br>
>> > runToolOnCode(Action.get(), "class X {};", ...);<br>
>> ><br>
>> > To me this would be the cleanest way to express ownership.<br>
>><br>
>> I'm not sure what you're implying here, but if that code change were<br>
>> made today it would lead to a double delete. runToolOnCode takes<br>
>> ownership (because it takes the parameter and, ultimately, returns it<br>
>> from FrontendActionFactory::create, which returns ownership). Are you<br>
>> implying some mechanism that would lead to runToolOnCode not taking<br>
>> ownership of its parameter? How?<br>
><br>
><br>
> Ah, you're right, I missed that. We could of course wrap the FrontendAction<br>
> (and forward the calls), but that seems like a bad solution. I take back<br>
> everything I said and claim the opposite ;) (well, not everything...)<br>
<br>
</div></div>If a FrontendAction could be wrapped and reused/returned from multiple<br>
create() calls, we wouldn't need a factory - we could just use a<br>
single FrontendAction all the time. That'd simplify things a bit.<br>
<div><div class="h5"><br>
>> I don't know this code very well, I was simply propagating the<br>
>> ownership semantics that were present in the code, and making them<br>
>> explicit in the type system. The code change I suggested reflects the<br>
>> existing ownership semantics of the code.<br>
>><br>
>> Either we should commit what I've suggested (enshrining the existing<br>
>> ownership semantics into the type system, making them more obvious) or<br>
>> we need to change the semantics. I don't know what kind of change in<br>
>> semantics would be appropriate, but I'm open to<br>
>> suggestions/discussion...<br>
>><br>
>> ><br>
>> >><br>
>> >> In<br>
>> >> any case, like I said - I was just changing the API to match the<br>
>> >> semantics as they stand today. This is already the API contract - and<br>
>> >> correct callers are already meeting this contract by passing a<br>
>> >> 'release()'d unique_ptr to runTool* functions. Changing it to<br>
>> >> unique_ptr just makes that more explicit/clear/less error-prone. If<br>
>> >> there's some other API design that fixes this in some other way, I'm<br>
>> >> OK with that - don't really mind.<br>
>> >><br>
>> >> >  but<br>
>> >> > I still am torn, because I value the idea that you can use the same<br>
>> >> > Action<br>
>> >> > for multiple runTool* calls.<br>
>> >> ><br>
>> >> > The original question was about the FrontendActionFactory objects<br>
>> >> > though<br>
>> >> > -<br>
>> >> > here I am still convinced that our interfaces should not take<br>
>> >> > ownership.<br>
>> >><br>
>> >> I'm confused - I haven't been advocating for FrontendActionFactories<br>
>> >> to have ownership passed to Tool.run. My change only changed Tool.run<br>
>> >> from taking a non-owning pointer to a non-owning reference, in both<br>
>> >> cases the callee owns the factory.<br>
>> >><br>
>> >> My patch was just to demonstrate an answer to Richard Smith's question:<br>
>> >><br>
>> >> "Why do we need to heap-allocate the FrontendActionFactory at all?"<br>
>> >><br>
>> >> (which is to say, by way of example, "we don't need to heap-allocate<br>
>> >> the FrontendActionFactory at all")<br>
>> ><br>
>> ><br>
>> > well, "for the use cases that are in the clang codebase". To me,<br>
>> > interfaces<br>
>> > are about making simple things simple for users. If you can patch it<br>
>> > into<br>
>> > our codebase and make the reverse deps work without making the client<br>
>> > code<br>
>> > significantly more complex, I'm not opposed to the change (but we might<br>
>> > argue about what "more complex" means ...)<br>
>><br>
>> Working on it.<br>
><br>
><br>
> Cool, looking forward to taking a look at the change. Thanks!<br>
<br>
</div></div>Attached the latest patches that preserve the factory factory<br>
functions. The reason I asked about those is that they seem almost a<br>
bit excessive in a world where they're not polymorphically<br>
owning/allocated. Especially the SimpleFrontendActionFactory - why<br>
would that need a factory function to build it? So I figured maybe<br>
it'd be nice to drop the newFrontendActionFactory functions in the<br>
process, if that's the appropriate design currently (even if it means<br>
more churn) rather than keeping a backwards compatible-ish API for the<br>
sake of lower churn.<br>
<br>
But if you feel it's the right design (to keep the factory builder<br>
helper functions) irrespective of the history here, that's your call -<br>
I don't feel strongly about it. I've attached patches that go that<br>
way, just having newFrontendActionFactory functions return by value<br>
instead of by unique_ptr.<br>
<br>
Does that look like what you were thinking of? Are you OK with a<br>
single commit, or would you like it broken up in any particular ways?<br>
(I have it in a few stages in git, but they're a bit jumbled/would<br>
take some work to pry them out into nicely modular commits)<br>
<br>
(you can see in the internal code review the matching changes internally)<br></blockquote><div><br></div><div>Would you mind uploading them to phab for review?</div><div><br></div><div>Thx!</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 class="HOEnZb"><font color="#888888"><br>
- David<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
><br>
>><br>
>><br>
>> - David<br>
>><br>
>> ><br>
>> > Cheers,<br>
>> > /Manuel<br>
>> ><br>
>> >><br>
>> >><br>
>> >> - David<br>
>> >><br>
>> >><br>
>> >> ><br>
>> >> > Cheers,<br>
>> >> > /Manuel<br>
>> >> ><br>
>> >> ><br>
>> >> >><br>
>> >> >><br>
>> >> >> - David<br>
>> >> >><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<br>
>> >> >> >> >> > one<br>
>> >> >> >> >> > 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)<br>
>> >> >> >> >> local<br>
>> >> >> >> >> variable and pass it to each run call - the object would never<br>
>> >> >> >> >> move<br>
>> >> >> >> >> around, so pointers to it would remain valid throughout its<br>
>> >> >> >> >> lifetime.<br>
>> >> >> >> >><br>
>> >> >> >> >> Or am I picturing something different from what you have in<br>
>> >> >> >> >> 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>
>> >> >> ><br>
>> >> >> ><br>
>> >> ><br>
>> >> ><br>
>> ><br>
>> ><br>
><br>
><br>
</div></div></blockquote></div><br></div></div>