[PATCH][tablegen] Eliminate memory leaks in TGParser.cpp
Anton Yartsev
anton.yartsev at gmail.com
Wed Aug 6 18:24:18 PDT 2014
On 07.08.2014 4:46, Sean Silva wrote:
>
>
>
> On Wed, Aug 6, 2014 at 5:12 PM, Anton Yartsev <anton.yartsev at gmail.com
> <mailto:anton.yartsev at gmail.com>> wrote:
>
> On 07.08.2014 1:01, David Blaikie wrote:
>
> On Wed, Aug 6, 2014 at 1:46 PM, Anton Yartsev
> <anton.yartsev at gmail.com <mailto:anton.yartsev at gmail.com>> wrote:
>
> On 06.08.2014 20:39, David Blaikie wrote:
>
> On Tue, Aug 5, 2014 at 8:50 PM, Anton Yartsev
> <anton.yartsev at gmail.com <mailto:anton.yartsev at gmail.com>>
> wrote:
>
> Got it. Attached is an updated patch. First tried
> to use unique_ptr but
> soon
> realized that shared_ptr is more suitable here.
>
> Hmm - let's have a bit of a chat about this
> (shared_ptr usually makes
> me twitch a bit - I strongly prefer simpler ownership
> (as it makes the
> code easier to understand where it can be applied).
>
> What makes shared_ptr more suitable? From what I could
> see in your
> first (and this) patch the code looked something like
> this:
>
> owner = new T;
> container.add(owner); // pass ownership here)
> process(owner); // do stuff with the T, but the
> container will
> guarantee its survival here, this isn't shared ownership
>
> If the shared_ptrs that are outside the container
> never themselves
> result in the object being destroyed (eg: only during
> the the
> destruction of the shared_ptrs in the container are
> the T objects ever
> destroyed) then I don't think this is shared ownership.
>
>
> I found shared_ptr handy for situation when the ownership
> is not guarantee
> to be transferred.
>
> owner = new T;
> if (/*condition*/)
> container.add(owner); // pass ownership here
> process(owner);
>
> It still seems like there's a single owner for this object at any
> given point - so using shared_ptr seems misleading to me.
>
> Also - where does this conditional ownership passing crop-up
> in your
> patch? (just so I can more easily go & have a look at it in-situ)
>
> Look at bool TGParser::ParseDef(MultiClass *CurMultiClass);
> Attached is a full diff.
> 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.
> It seems that it is better just to manually call delete where
> needed. Should I move in this direction?
>
>
> 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.
>
> 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.
Attached is the patch with the raw delete calls added. Just here appears
the flag indicating whether the ownership was transferred or not..
>
> -- Sean Silva
>
>
>
> Using smart pointer with reference counter here just
> simply guarantee the
> memory to be released if the ownership was not taken. With
> shared_ptr it is
> also no need to involve additional variable for local
> usage what simplifies
> the code IMHO.
>
> It simplifies it in some ways but complicates it in others.
> Personally, I'd avoid this use of shared_ptr, but I imagine
> opinions
> vary on this so don't necessarily take my word for it.
>
> - David
>
>
>
> I'll followup to Sean's reply with some more specific
> thoughts there.
>
> Personally I'd favor:
>
> auto foo = llvm::make_unique<T>(...);
> foo = llvm::make_unique<T>(...);
>
> over:
>
> std::unique_ptr<T> foo(new T(...));
> foo.reset(new T(...));
>
> Though opinions may vary there.
>
> I'd probably consider renaming your
> "CurRecLocalView" to "CurRec" so
> that most of the code doesn't need to change
> (and CurRecUniquePtr
> maybe "CurRecOwner"?)
>
> Also in a preliminary/prior patch, it might be
> worth changing "addDef"
> to take a unique_ptr by value to properly
> document the ownershp
> passing (I'm hoping to keep focusing on any
> 'release()' calls and use
> them as a marker of places to clean
> up/continue pushing unique_ptr
> through).
>
> On Tue, Aug 5, 2014 at 12:20 PM, Anton Yartsev
> <anton.yartsev at gmail.com
> <mailto:anton.yartsev at gmail.com>>
> wrote:
>
> Hi all,
>
> I've recently ran the
> alpha.cplusplus.NewDeleteLeaks checker
> over the
> LLVM
> codebase to eliminate false-positives. The
> checker evolved a number of
> real
> leaks. I've decided to fix some of them.
> Attached is the patch that eliminates
> memory leaks found in the
> TGParser.cpp. Please review.
>
> --
> Anton
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> <mailto:llvm-commits at cs.uiuc.edu>
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
> --
> Anton
>
>
> --
> Anton
>
>
>
> --
> Anton
>
>
--
Anton
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140807/68655ff0/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: TGParser_eliminate_leaks_v02_full.patch
Type: text/x-diff
Size: 87231 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140807/68655ff0/attachment.patch>
More information about the llvm-commits
mailing list