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

[David Blaikie]

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