[PATCH] Own/produce FrontendActionFactories by value, rather than new'd/unique_ptr ownership.

David Blaikie dblaikie at gmail.com
Tue Sep 16 09:59:58 PDT 2014


On Tue, Sep 16, 2014 at 9:51 AM, Manuel Klimek <klimek at google.com> wrote:

>
>  On Tue, Sep 16, 2014, 00:23 David Blaikie <dblaikie at gmail.com> wrote:
>
> >>! In D4313#12, @klimek wrote:
> > This has now been resolved differently, right? Can we abandon the
> revision?
>
> Not exactly - r207396 made the newFrontendActionFactory change to return
> unique_ptr, but the original memory leak could still easily be written
> because it takes a raw pointer (this both complicates callers (having them
> either use "newFrontendActionFactory(...).get()" or take the address of a
> local to pass) and risks someone passing a raw 'new' to the function)).
>
>  Isn't that just DI?
>
DI? Dependency Injection? All I'm suggesting is the difference between
passing by reference and passing by pointer. It shouldn't change the
semantics being expressed here (except to clarify that this is not passing
ownership).

> Are you suggesting to not use DI any more just because somebody might hand
> in an unowned pointer?
>
No, just suggesting to use references instead.

>
> The follow-on refactor to have newFrontendActionFactory return by value (&
> then, potentially, remove its virtual dtor) is just to simplify the call
> sites even further. Rather than replacing
> "run(newFrontendActionFactory(...).get())" with
> "run(*newFrontendActionFactory(...))" (which might make people twitch a
> bit, wondering whether that leaks or not) it could just be
> "run(newFrontendActionFactory(...))" which seems less surprising.
>
> I figure if I'm going to have to touch every caller anyway, it wasn't
> really extra churn to change newFrontendActionFactory to be value-based.
>
> And there's still the matter of runToolOnCode taking ownership via raw
> pointer (which then feeds into that confusion we had over ToolInvocation
> needing to take ownership and SingleFrontendActionFactory taking/returning
> ownership of that FrontendActionFactory). But that's fairly separable.
>
> http://reviews.llvm.org/D4313
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140916/658058d8/attachment.html>


More information about the cfe-commits mailing list