On Wed, Sep 17, 2014 at 5:32 AM, Manuel Klimek <[email protected]> wrote:
> > On Tue, Sep 16, 2014, 21:36 David Blaikie <[email protected]> wrote: > > On Tue, Sep 16, 2014 at 11:56 AM, Manuel Klimek <[email protected]> wrote: > > > On Tue, Sep 16, 2014, 18:59 David Blaikie <[email protected]> wrote: > > On Tue, Sep 16, 2014 at 9:51 AM, Manuel Klimek <[email protected]> wrote: > > > > On Tue, Sep 16, 2014, 00:23 David Blaikie <[email protected]> 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 > > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
