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

[Anton Yartsev]

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

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.
  

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