<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Aug 6, 2014 at 5:12 PM, Anton Yartsev <span dir="ltr"><<a href="mailto:anton.yartsev@gmail.com" target="_blank">anton.yartsev@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On 07.08.2014 1:01, David Blaikie wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On Wed, Aug 6, 2014 at 1:46 PM, Anton Yartsev <<a href="mailto:anton.yartsev@gmail.com" target="_blank">anton.yartsev@gmail.com</a>> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On 06.08.2014 20:39, David Blaikie wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On Tue, Aug 5, 2014 at 8:50 PM, Anton Yartsev <<a href="mailto:anton.yartsev@gmail.com" target="_blank">anton.yartsev@gmail.com</a>><br>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Got it. Attached is an updated patch. First tried to use unique_ptr but<br>
soon<br>
realized that shared_ptr is more suitable here.<br>
</blockquote>
Hmm - let's have a bit of a chat about this (shared_ptr usually makes<br>
me twitch a bit - I strongly prefer simpler ownership (as it makes the<br>
code easier to understand where it can be applied).<br>
<br>
What makes shared_ptr more suitable? From what I could see in your<br>
first (and this) patch the code looked something like this:<br>
<br>
owner = new T;<br>
container.add(owner); // pass ownership here)<br>
process(owner); // do stuff with the T, but the container will<br>
guarantee its survival here, this isn't shared ownership<br>
<br>
If the shared_ptrs that are outside the container never themselves<br>
result in the object being destroyed (eg: only during the the<br>
destruction of the shared_ptrs in the container are the T objects ever<br>
destroyed) then I don't think this is shared ownership.<br>
</blockquote>
<br>
I found shared_ptr handy for situation when the ownership is not guarantee<br>
to be transferred.<br>
<br>
owner = new T;<br>
if (/*condition*/)<br>
container.add(owner); // pass ownership here<br>
process(owner);<br>
</blockquote>
It still seems like there's a single owner for this object at any<br>
given point - so using shared_ptr seems misleading to me.<br>
<br>
Also - where does this conditional ownership passing crop-up in your<br>
patch? (just so I can more easily go & have a look at it in-situ)<br>
</blockquote></div></div>
Look at bool TGParser::ParseDef(MultiClass *CurMultiClass); Attached is a full diff.<br>
Moreover, with solution that uses unique_ptr an additional flag (or other trick) is required to indicate if the ownership was transferred or not to know if we should free the memory at returns.<br>
It seems that it is better just to manually call delete where needed. Should I move in this direction?</blockquote><div><br></div><div>I'm starting to think that just adding the necessary delete calls is the most pragmatic thing to do (how many are needed?). It seems like a more thorough refactoring of the ownership here is needed that goes beyond the scope of fixing the leaks here.</div>
<div><br></div><div>Adding the raw delete calls is also at least in line with the philosophy of "every naked new and delete is a red flag", clearly indicating where the ownership situation needs to be improved for future developers.</div>
<div><br></div><div>-- Sean Silva</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"><br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Using smart pointer with reference counter here just simply guarantee the<br>
memory to be released if the ownership was not taken. With shared_ptr it is<br>
also no need to involve additional variable for local usage what simplifies<br>
the code IMHO.<br>
</blockquote>
It simplifies it in some ways but complicates it in others.<br>
Personally, I'd avoid this use of shared_ptr, but I imagine opinions<br>
vary on this so don't necessarily take my word for it.<br>
<br>
- David<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
I'll followup to Sean's reply with some more specific thoughts there.<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Personally I'd favor:<br>
<br>
auto foo = llvm::make_unique<T>(...);<br>
foo = llvm::make_unique<T>(...);<br>
<br>
over:<br>
<br>
std::unique_ptr<T> foo(new T(...));<br>
foo.reset(new T(...));<br>
<br>
Though opinions may vary there.<br>
<br>
I'd probably consider renaming your "CurRecLocalView" to "CurRec" so<br>
that most of the code doesn't need to change (and CurRecUniquePtr<br>
maybe "CurRecOwner"?)<br>
<br>
Also in a preliminary/prior patch, it might be worth changing "addDef"<br>
to take a unique_ptr by value to properly document the ownershp<br>
passing (I'm hoping to keep focusing on any 'release()' calls and use<br>
them as a marker of places to clean up/continue pushing unique_ptr<br>
through).<br>
<br>
On Tue, Aug 5, 2014 at 12:20 PM, Anton Yartsev <<a href="mailto:anton.yartsev@gmail.com" target="_blank">anton.yartsev@gmail.com</a>><br>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi all,<br>
<br>
I've recently ran the alpha.cplusplus.NewDeleteLeaks checker over the<br>
LLVM<br>
codebase to eliminate false-positives. The checker evolved a number of<br>
real<br>
leaks. I've decided to fix some of them.<br>
Attached is the patch that eliminates memory leaks found in the<br>
TGParser.cpp. Please review.<br>
<br>
--<br>
Anton<br>
<br>
<br>
______________________________<u></u>_________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/llvm-commits</a><br>
<br>
</blockquote></blockquote>
--<br>
Anton<br>
<br>
</blockquote></blockquote>
<br>
--<br>
Anton<br>
<br>
</blockquote></blockquote>
<br>
<br></div></div><span class="HOEnZb"><font color="#888888">
-- <br>
Anton<br>
<br>
</font></span></blockquote></div><br></div></div>