[patch] Don't own the buffer in object::Binary

[Rafael Espíndola]

Rebased patch attached.

On 14 August 2014 22:36, Rafael Espíndola <rafael.espindola at gmail.com> wrote:
> On 14 August 2014 16:42, Lang Hames <lhames at gmail.com> wrote:
>> This looks really great.
>>
>> Regarding MemoryBufferRef - ideally I think should look like an
>> ArrayRef<uint8_t> with a name. That's orthogonal to what you're trying to do
>> in this patch though, and we can discuss it separately.
>
> Yes, but for now something that looks a bit like MemoryBuffer makes
> the transition simpler.
>
>> I think the C API should wrap OwningBinary to preserve the existing
>> semantics for now. We can also put a message out on the dev list and ask for
>> feedback from the existing users of the API and see if they'd prefer a
>> change.
>
> Done. Once this is in we can have another thread on how to change the C API.
>
>> Apart from that I've just got a couple of notes on the patch itself:
>>
>> The comment on makeLTOModule says that it takes ownership of the buffer, but
>> the new signature suggests otherwise. I think the comment should be updated?
>
> Fixed.
>
>> I think the changes to ObjectFileCoverageMappingReader are incorrect: An
>> OwningBinary is created, but then the non-owning Binary is extracted, and
>> the handle to the backing memory buffer is lost. I think
>> ObjectFileCoverageMappingReader's Object member needs to be changed from a
>> std::unique_ptr<llvm::object::ObjectFile> to an
>> llvm::object::OwningBinary<llvm::object::ObjectFile>.
>
> Fixed.
>
>> Everything else looks good to me.
>
> Awesome. Attached is a new version of the patch. Please take another look.
>
> Cheers,
> Rafael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: t.patch
Type: text/x-patch
Size: 71317 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140817/9172b3bd/attachment.bin>