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

[David Blaikie]

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.

- 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/20140916/606abd8c/attachment.html>