<div dir="ltr">This is ugly but I think it is clearer than all the other patches. All the naked delete's are a red flag for future developers. Eventually as this code is refactored they will go away (also they can *guide* future refactoring). At this point, it seems like a nontrivial refactoring is necessary to get the ownership here under control, so I think this patch makes sense for now. LGTM. Please wait for David to give his LGTM as well.<div>
<div><br></div><div>-- Sean Silva</div></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Aug 6, 2014 at 6:24 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 text="#000000" bgcolor="#FFFFFF"><div><div class="h5">
<div>On 07.08.2014 4:46, Sean Silva wrote:<br>
</div>
<blockquote type="cite">
<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>
<div>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>
</div>
</div>
</blockquote></div></div>
Attached is the patch with the raw delete calls added. Just here
appears the flag indicating whether the ownership was transferred or
not..<div><div class="h5"><br>
<br>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<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>
<div><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>
_______________________________________________<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/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><font color="#888888">
-- <br>
Anton<br>
<br>
</font></span></blockquote>
</div>
<br>
</div>
</div>
</blockquote>
<br>
<br>
</div></div><span class="HOEnZb"><font color="#888888"><pre cols="72">--
Anton</pre>
</font></span></div>
</blockquote></div><br></div>