[llvm] r211184 - Fix a memory leak in the error path.

[Rafael Espíndola]

>> Well, I would say that with this patch there is no conditional
>> ownership any more. The buffer is moved from one owner to another.
>
> Except it isn't. In the example I looked at (
> llvm-ar.cpp::writeSymbolTable ) the caller still, for all intents and
> purposes, still seems to have ownership - the pointers are still in
> the vector and will still be deleted (indeed they might be replaced
> with unique_ptrs in the vector at some point). It's not really
> transferring ownership - it's pretending to. If the release never
> occurred we wouldn't be in a state where the ObjectFile owned the
> thing and the caller didn't, instead we'd be in the state where both
> of them own it and a double delete would occur.
>
> For example, to properly represent this ownership semantic,
> llvm-ar.cpp::writeSymbolTable would have to take a
> MutableArrayRef<unique_ptr<T>> rather than an ArrayRef<T*> - even
> though the sequence /should/ be the same at the end of the function
> call as at the beginning, because in the middle it'll move things out
> of the sequence, then move them back... that doesn't seem like it
> really reflects the contract of that function in the type system (we
> have to make something mutable, which, semantically, it isn't).

OK, I agree we are still not at the stage of always explicitly passing
ownership, but this should be a step in the right direction, no?

> If that were really what the callers wanted to do, I might be inclined
> to agree - but it seems like the callers honestly never want the
> ObjectFile to own the thing. They will unconditionally move it back
> again anyway - but I'd be happier (if not exactly 'happy') with this
> if it were actually moved around and the ownership transfer were more
> clear, but still suspicious of it.

Having ObjectFile own the buffer seems to be the more common case
actually. Same goes for the IR reader.

>>
>>> I mean I guess that's sort of equivalent to the world we're in today,
>>> but hopefully separates concerns better. It does still make something
>>> dynamic where it was static, but the only way to keep it static would
>>> be for the whole hierarchy of ObjectFiles, their factories, etc,
>>> templated, I think?
>>>
>>
>> I would like to avoid the extra templates if possible.
>
> Likewise - I'm just trying to enumerate the options that are possible
> to make sure I haven't missed anything, etc.

OK.

IMHO the patch as is in already an improvement since with it no object
ever has to keep a distinct boolean saying it owns the buffer. I will
try to update the patch a bit further down the unique_ptr path by
changing the create methods as you suggested.

Cheers,
Rafael