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

[David Blaikie]

On Wed, Sep 17, 2014 at 5:32 AM, Manuel Klimek <klimek at google.com> wrote:

>
>  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) ;
>
Yep, nothing wrong with any of that (though most users won't need to
dynamically own/allocate factories - coming back to that discussion about
virtual dtor - but that's a separate thing to discuss/do after everything
else)

> As long as faf stays an interface...
>
 Not sure I'm following what you mean by this comment - what else would it
be?

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