<div dir="ltr">Thanks Anton for checking out these leaks.<br><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Aug 5, 2014 at 3:28 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br>
<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></blockquote><div><br></div><div>I think this suggestion is quite a bit cleaner.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<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></blockquote><div><br></div><div>I found the CurRecLocalView to be confusing. Why is it needed? Adding tons of explicit .get()'s (and .release()) is a nice way to indicate that this code still needs more work to get its ownership situation cleaned up. Hiding that with a local variable just... hides it.</div>
<div><br></div><div>Is it possible to just use a single variable `std::unique_ptr<Record> CurRec;` and tons of .get()'s and .release()'s? That also ensures a "single point of truth".</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">
<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>
<div><div class="h5"><br>
On Tue, Aug 5, 2014 at 12:20 PM, Anton Yartsev <<a href="mailto:anton.yartsev@gmail.com">anton.yartsev@gmail.com</a>> wrote:<br>
> Hi all,<br>
><br>
> I've recently ran the alpha.cplusplus.NewDeleteLeaks checker over the LLVM<br>
> codebase to eliminate false-positives. The checker evolved a number of 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>
</div></div>> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu">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>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">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>
</blockquote></div><br></div></div>