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

[Manuel Klimek]

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

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


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)


Given "run(new T())" and "run(*new T())" the former looks totally plausible
(well, plausible-ish, until we live in a world where almost everything is
unique_ptr'd) while the latter looks rather suspicious/errouneous.

But, yes, part of this is motivated by more idiomatic LLVM code (though
LLVM's hardly consistent on the pass-by-reference idea, so I can't make a
strong claim there)

So then the question is - if I'm updating all the call sites anyway, I
could update the factories to return by value and then it'd go from
"run(newFrontendActionFactory(...).get())" to
"run(newFrontendActionFactory(...))" instead of to
"run(*newFrontendActionFactory(...))" (though at that point, perhaps we'd
want to drop the "new" and replace it with "make" (since it's not just a
template argument deduction helper) or something else that indicates that
it's not returning new'd memory). This makes all the callers simpler except
for that one ClangCheck which gets difficult - which would either need the
minor code duplication, or the other options we discussed previously.

 If run doesn't take ownership any more, it seems like
SpecificFrontendActionFactory factory;
run(factory);
Should be what we use in most places.

In other places it still seems like we could do


 Unique_ptr <FrontendActionFactory > factory ;


 // assign factory
run(*factory) ;

As long as faf stays an interface...

- David


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/20140917/052766ec/attachment.html>