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

[David Blaikie]

On Tue, Aug 5, 2014 at 6:04 PM, Sean Silva <chisophugis at gmail.com> wrote:
> Thanks Anton for checking out these leaks.
>
>
> On Tue, Aug 5, 2014 at 3:28 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>> 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 think this suggestion is quite a bit cleaner.
>
>>
>> I'd probably consider renaming your "CurRecLocalView" to "CurRec" so
>> that most of the code doesn't need to change (and CurRecUniquePtr
>> maybe "CurRecOwner"?)
>
>
> I found the CurRecLocalView to be confusing. Why is it needed? Adding tons
> of explicit .get()'s (and .release()) is a nice way to indicate that this
> code still needs more work to get its ownership situation cleaned up. Hiding
> that with a local variable just... hides it.

I don't think it's quite hiding anything, really - so far as I can
tell from the code we've got some code that boils down to:

  T *t = new T();
  container.add(t); // ownership passed here
  process(t); // process doesn't own the T, container owns it over the
duration of 'process'

Assuming 'process' needs to be called with 't' in the container,
there's not much to this - you're going to need a non-owning and
owning reference to T when you transform this into explicit ownership
code:

  auto t = make_unique<T>();
  T &non_owning_t = *t;
  container.add(t.release()); // or 'container.add(std::move(t))' if
you fix container::add's ownership passing to be explicit
  process(&t); // or fix 'process' to take T& instead of T*, if
possible (if it doesn't need optionality, etc) so it's just
'process(t)'

About the only other way I can imagine this is if you changed
container.add to return a reference:

  T &t = container.add(make_unique<T>());
  process(t);

But that only works if you only have one place to make a T, and I
think this code has several different codepaths - could be factored
into some factory function, I suppose.

> Is it possible to just use a single variable `std::unique_ptr<Record>
> CurRec;` and tons of .get()'s and .release()'s? That also ensures a "single
> point of truth".

I suspect not, as described above - non-owning uses are required after
ownership is transferred into a container.

Anton - is this an accurate representation of the ownership in this code?

- David

>
> -- Sean Silva
>
>>
>>
>> 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
>> >
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>