<div dir="ltr"><div>This looks really great.</div><div><br></div><div>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.</div>
<div><br></div><div>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.</div>
<div><br></div><div>Apart from that I've just got a couple of notes on the patch itself:</div><div><br></div><div><div>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?</div>
<div><br></div><div>
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>. </div>
<div><br></div><div>Everything else looks good to me.</div><div><br></div><div>Cheers,</div><div>Lang.</div><div><br></div></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Aug 12, 2014 at 5:08 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">ping. A rebased patch is attached.<br>
<div class="HOEnZb"><div class="h5"><br>
On 6 August 2014 18:00, Rafael Espíndola <<a href="mailto:rafael.espindola@gmail.com">rafael.espindola@gmail.com</a>> wrote:<br>
> An updated version is attached.<br>
><br>
> The main difference is that now Object keeps a reference both to the<br>
> data and to the name instead of copying the name to a std::string. I<br>
> also changed Archive to provide MemoryBufferRef to access its members<br>
> instead of MemoryBuffers.<br>
><br>
> I think this is in good enough shape for a code review now :-)<br>
><br>
><br>
> On 1 August 2014 16:32, Rafael Espíndola <<a href="mailto:rafael.espindola@gmail.com">rafael.espindola@gmail.com</a>> wrote:<br>
>> There were quiet a few requests for not owning the buffer in<br>
>> object::Binary, this an attempt to make that happen (and make it<br>
>> obvious in the API).<br>
>><br>
>> The patch is intentionally focused on lib/Object. If some other<br>
>> structure had a std::unique_ptr<Binary>, now it also has a<br>
>> std::unique_ptr<MemoryBuffer>.<br>
>><br>
>> The main things It would be nice to have some feedback on:<br>
>><br>
>> * MemoryBufferRef. Should this have another name? Should it implement<br>
>> more of the MemoryBuffer API instead?<br>
>><br>
>> * OwningBinary: We need something like this for the convenience<br>
>> methods that construct an ObjectFile from a file name.<br>
>><br>
>> * What should we do about the C API? Should it start wrapping<br>
>> OwningBinary instead of Binary to keep the existing semantics? Should<br>
>> we put a big warning in the 3.6 release and change it (I don't expect<br>
>> too many users at this point)?<br>
>><br>
>> * There is still a fixme about removing getMemoryBuffer from Archive.<br>
>> I should have an updated patch with that early next weak, but I don't<br>
>> expect it to change anything in a fundamental way.<br>
>><br>
>> I am still running the tests with Valgrind, but we can probably start<br>
>> the review in parallel.<br>
>><br>
>> Cheers,<br>
>> Rafael<br>
</div></div></blockquote></div><br></div>