[PATCH][tablegen] Eliminate memory leaks in TGParser.cpp

[Anton Yartsev]

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> 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>
>>> 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?
>
>> 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>
>>>>> 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
>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>>>
>>>> --
>>>> Anton
>>>>
>>
>> --
>> Anton
>>


-- 
Anton

-------------- next part --------------
A non-text attachment was scrubbed...
Name: TGParser_eliminate_leaks_v01_full.patch
Type: text/x-diff
Size: 237176 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140807/8ad1fb39/attachment.patch>