<div dir="ltr">Looks good to me. Thanks Rafael!</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Aug 18, 2014 at 2:07 PM, Rafael Espíndola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Lang,<br>
<br>
I realized that ObjectBuffer::getMemBuffer can new return just a<br>
MemoryBufferRef. New version attached.<br>
<div class="HOEnZb"><div class="h5"><br>
<br>
<br>
On 17 August 2014 17:01, Rafael Espíndola <<a href="mailto:rafael.espindola@gmail.com">rafael.espindola@gmail.com</a>> wrote:<br>
> Rebased patch attached.<br>
><br>
> On 14 August 2014 22:36, Rafael Espíndola <<a href="mailto:rafael.espindola@gmail.com">rafael.espindola@gmail.com</a>> wrote:<br>
>> On 14 August 2014 16:42, Lang Hames <<a href="mailto:lhames@gmail.com">lhames@gmail.com</a>> wrote:<br>
>>> This looks really great.<br>
>>><br>
>>> Regarding MemoryBufferRef - ideally I think should look like an<br>
>>> ArrayRef<uint8_t> with a name. That's orthogonal to what you're trying to do<br>
>>> in this patch though, and we can discuss it separately.<br>
>><br>
>> Yes, but for now something that looks a bit like MemoryBuffer makes<br>
>> the transition simpler.<br>
>><br>
>>> I think the C API should wrap OwningBinary to preserve the existing<br>
>>> semantics for now. We can also put a message out on the dev list and ask for<br>
>>> feedback from the existing users of the API and see if they'd prefer a<br>
>>> change.<br>
>><br>
>> Done. Once this is in we can have another thread on how to change the C API.<br>
>><br>
>>> Apart from that I've just got a couple of notes on the patch itself:<br>
>>><br>
>>> The comment on makeLTOModule says that it takes ownership of the buffer, but<br>
>>> the new signature suggests otherwise. I think the comment should be updated?<br>
>><br>
>> Fixed.<br>
>><br>
>>> I think the changes to ObjectFileCoverageMappingReader are incorrect: An<br>
>>> OwningBinary is created, but then the non-owning Binary is extracted, and<br>
>>> the handle to the backing memory buffer is lost. I think<br>
>>> ObjectFileCoverageMappingReader's Object member needs to be changed from a<br>
>>> std::unique_ptr<llvm::object::ObjectFile> to an<br>
>>> llvm::object::OwningBinary<llvm::object::ObjectFile>.<br>
>><br>
>> Fixed.<br>
>><br>
>>> Everything else looks good to me.<br>
>><br>
>> Awesome. Attached is a new version of the patch. Please take another look.<br>
>><br>
>> Cheers,<br>
>> Rafael<br>
</div></div></blockquote></div><br></div>