<p dir="ltr"><br>
</p>
<p dir="ltr"></p>
<p dir="ltr">On Tue, Sep 16, 2014, 00:23 David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:</p>
<blockquote><p dir="ltr">>>! In D4313#12, @klimek wrote:<br>
> This has now been resolved differently, right? Can we abandon the revision?</p>
<p dir="ltr">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)).<br>
</p>
</blockquote>
<p dir="ltr"></p>
<p dir="ltr">Isn't that just DI? Are you suggesting to not use DI any more just because somebody might hand in an unowned pointer? </p>
<blockquote><p dir="ltr"><br></p>
<p dir="ltr">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.</p>
<p dir="ltr">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.<br><br></p>
<p dir="ltr">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.</p>
<p dir="ltr"><a href="http://reviews.llvm.org/D4313">http://reviews.llvm.org/D4313</a></p>
</blockquote>
<p dir="ltr"><br>
</p>