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

[Lang Hames]

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.

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.

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?

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>.

Everything else looks good to me.

Cheers,
Lang.



On Tue, Aug 12, 2014 at 5:08 PM, Rafael Espíndola <
rafael.espindola at gmail.com> wrote:

> ping. A rebased patch is attached.
>
> On 6 August 2014 18:00, Rafael Espíndola <rafael.espindola at gmail.com>
> wrote:
> > An updated version is attached.
> >
> > The main difference is that now Object keeps a reference both to the
> > data and to the name instead of copying the name to a std::string. I
> > also changed Archive to provide MemoryBufferRef to access its members
> > instead of MemoryBuffers.
> >
> > I think this is in good enough shape for a code review now :-)
> >
> >
> > On 1 August 2014 16:32, Rafael Espíndola <rafael.espindola at gmail.com>
> wrote:
> >> There were quiet a few requests for not owning the buffer in
> >> object::Binary, this an attempt to make that happen (and make it
> >> obvious in the API).
> >>
> >> The patch is intentionally focused on lib/Object. If some other
> >> structure had a std::unique_ptr<Binary>, now it also has a
> >> std::unique_ptr<MemoryBuffer>.
> >>
> >> The main things It would be nice to have some feedback on:
> >>
> >> * MemoryBufferRef. Should this have another name? Should it implement
> >> more of the MemoryBuffer API instead?
> >>
> >> * OwningBinary: We need something like this for the convenience
> >> methods that construct an ObjectFile from a file name.
> >>
> >> * What should we do about the C API? Should it start wrapping
> >> OwningBinary instead of Binary to keep the existing semantics? Should
> >> we put a big warning in the 3.6 release and change it (I don't expect
> >> too many users at this point)?
> >>
> >> * There is still a fixme about removing getMemoryBuffer from Archive.
> >> I should have an updated patch with that early next weak, but I don't
> >> expect it to change anything in a fundamental way.
> >>
> >> I am still running the tests with Valgrind, but we can probably start
> >> the review in parallel.
> >>
> >> Cheers,
> >> Rafael
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140814/b16d5820/attachment.html>