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

[David Blaikie]

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)

>
> 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
>