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. - 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
