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

[Manuel Klimek]

 On Tue, Sep 16, 2014, 18:59 David Blaikie <dblaikie at gmail.com> wrote:

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.

 Ah sure using references sounds fine. I was mainly confused because that
doesn't seem to significantly change how easy it is to write a memory leak
(but sure fits llvm style better)


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/a67a4485/attachment.html>