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

[Rafael Espíndola]

Hi Lang,

I realized that ObjectBuffer::getMemBuffer can new return just a
MemoryBufferRef. New version attached.



On 17 August 2014 17:01, Rafael Espíndola <rafael.espindola at gmail.com> wrote:
> 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: 80329 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140818/c2a3b36e/attachment.bin>