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

[Lang Hames]

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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140819/1afb58dd/attachment.html>