<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Sep 16, 2014 at 11:56 AM, Manuel Klimek <span dir="ltr"><<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""><p dir="ltr"><br>
</p>
<p dir="ltr"></p>
<p dir="ltr">On Tue, Sep 16, 2014, 18:59 David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</p>
<blockquote><p dir="ltr">On Tue, Sep 16, 2014 at 9:51 AM, Manuel Klimek <<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>> wrote:</p>
</blockquote>
<blockquote><blockquote><p dir="ltr"><br>
On Tue, Sep 16, 2014, 00:23 David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</p>
</blockquote>
</blockquote>
<blockquote><blockquote><blockquote><p dir="ltr">>>! In D4313#12, @klimek wrote:<br>
> This has now been resolved differently, right? Can we abandon the revision?</p>
<p dir="ltr">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)).<br>
</p>
</blockquote>
</blockquote>
</blockquote>
<blockquote><blockquote><p dir="ltr"><br>
Isn't that just DI? <br>
</p>
</blockquote>
</blockquote>
<blockquote><p dir="ltr"><br>
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). </p>
</blockquote>
<blockquote><blockquote><p dir="ltr">Are you suggesting to not use DI any more just because somebody might hand in an unowned pointer?<br>
</p>
</blockquote>
</blockquote>
<blockquote><p dir="ltr"><br>
No, just suggesting to use references instead. </p>
</blockquote>
<p dir="ltr"></p>
</span><p dir="ltr">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)</p></blockquote><div>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.<br><br>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)<br><br>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.<br><br>- David<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><p dir="ltr"> </p><div class="HOEnZb"><div class="h5">
<blockquote><p dir="ltr"></p>
</blockquote>
<blockquote><blockquote><blockquote><p dir="ltr"><br>
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.</p>
<p dir="ltr">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.</p>
<p dir="ltr">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.</p>
<p dir="ltr"><a href="http://reviews.llvm.org/D4313" target="_blank">http://reviews.llvm.org/D4313</a><br>
</p>
</blockquote>
</blockquote>
</blockquote>
<blockquote><blockquote><p dir="ltr"><br>
</p>
</blockquote>
</blockquote>
<p dir="ltr"><br>
</p>
</div></div></blockquote></div><br></div></div>