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

[Rafael Espíndola]

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: 70734 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140814/ab8412fe/attachment.bin>