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

[Anton Yartsev]

On 07.08.2014 4:12, Anton Yartsev 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> 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.
Sorry, forget this :)
> It seems that it is better just to manually call delete where needed. 
> Should I move in this direction?
But the question is still actual.
>>
>>> 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