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

[Rafael Espíndola]

Thanks!

On 19 August 2014 13:28, Lang Hames <lhames at gmail.com> wrote:
> Looks good to me. Thanks Rafael!
>
>
> On Mon, Aug 18, 2014 at 2:07 PM, Rafael Espíndola
> <rafael.espindola at gmail.com> wrote:
>>
>> 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
>
>