<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Jun 3, 2014 at 12:16 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 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 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 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 changes<br>
> that don't provide significant enough value to pay for the cost. I am also<br>
> not sure which changes exactly we're now talking about - I think it 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 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>> wrote:<br>
>><br>
>> On Wed, May 7, 2014 at 4:46 AM, Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>> 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 <<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 <<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 <<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>
>> >> >> >> >>> >>> Tool.run(newFrontendActionFactory<MyPluginASTAction>());<br>
>> >> >> >> >>> >>><br>
>> >> >> >> >>> >>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~<br>
>> >> >> >> >>> >>><br>
>> >> >> >> >>> >>> It is because newFrontendActionFactory has been changed<br>
>> >> >> >> >>> >>> to<br>
>> >> >> >> >>> >>> work<br>
>> >> >> >> >>> >>> with<br>
>> >> >> >> >>> >>> std::unique_ptr. So if I change my code to<br>
>> >> >> >> >>> >>> return<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 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 work/is<br>
>> >> >> >> >>> > meant<br>
>> >> >> >> >>> > to<br>
>> >> >> >> >>> > work now...<br>
>> >> >> >> >>><br>
>> >> >> >> >>> The result of newFrontendActionFactory() used to be 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 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 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<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 a<br>
>> >> bit vague/wrong). But, yes - as you can see in the patches, the types<br>
>> >> can just be used directly.<br>
>> >><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<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 we?<br>
>> >> > We'd<br>
>> >> > need to get rid of the factories if we want to not use pointers (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 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 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 tree<br>
>> > is<br>
>> > that I don't think arguing based on what we find in Clang's tree (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 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 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 immovability).<br>
>> >> >><br>
>> >> >> > Sure, a<br>
>> >> >> > templated class would work (basically just instantiate the<br>
>> >> >> > FrontendActionFactoryAdapter), but the problem is that then 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 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 by<br>
>> >> >> reference.<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 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<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 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 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>
>> ><br>
>> ><br>
>> > The problem is that if the class is to be used / extended by users of 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 points<br>
>> > you<br>
>> > make. Since it is a library, I put a lot of emphasis on what interface a<br>
>> > potential user gets, how easy it is to write bugs against that 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 want<br>
> *external* users of an interface to keep polymorphical ownership of a 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 of<br>
> this specific interface.<br>
<br>
</div></div>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.</blockquote><div><br></div><div>The problem is that other methods don't limit what users of the library can do with their own objects, but the destructor does. We intend users to implement an interface, so taking the possibility away from them to do ownership the way they like it seems like a rather radical move.</div>
<div><br></div><div>I am with you for classes where we actually want to take ownership (let's assume we'd have a method for users to inject their own AST nodes, but we want to take ownership of the nodes they give us). In that case, I'd also vote for making the destructor protected.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
>> > - 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 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 rules<br>
>> > it<br>
>> > is so easy to shoot your foot off that even compiler engineers don't use<br>
>> > it<br>
>> > that way... Having a rule to always add a virtual dtor if a class 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 compiler<br>
>> > can<br>
>> > catch it).<br>
>><br>
>> >> > My main problem with the attached patches is actually that it 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 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, 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 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>
</div></div>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></blockquote><div><br></div><div>Ah, you're right, I missed that. We could of course wrap the FrontendAction (and forward the calls), but that seems like a bad solution. I take back everything I said and claim the opposite ;) (well, not everything...)</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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>
<div><div class="h5"><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 though<br>
>> > -<br>
>> > here I am still convinced that our interfaces should not take 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, interfaces<br>
> are about making simple things simple for users. If you can patch it into<br>
> our codebase and make the reverse deps work without making the client code<br>
> significantly more complex, I'm not opposed to the change (but we might<br>
> argue about what "more complex" means ...)<br>
<br>
</div></div>Working on it.<br></blockquote><div><br></div><div>Cool, looking forward to taking a look at the change. Thanks!</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><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) 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 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>
</div></div></blockquote></div><br></div></div>