<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Jul 23, 2013 at 7:12 AM, 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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">LGTM, but please also take a look at my (late, sorry) comments on the<br>
original patch.<br>
<div><div class="h5"><br></div></div></blockquote><div><br></div><div>Thanks for the review. Here are your original comments:</div><div><br></div><div><span style="font-family:arial,sans-serif;font-size:12.727272033691406px">>Unless you want to test unbuffered files too, it might be better to do</span><br style="font-family:arial,sans-serif;font-size:12.727272033691406px">
<span style="font-family:arial,sans-serif;font-size:12.727272033691406px">>something like</span><br style="font-family:arial,sans-serif;font-size:12.727272033691406px"><font face="arial, sans-serif">>{</font><br><span style="font-family:arial,sans-serif;font-size:12.727272033691406px">> raw_fd_ostream OF(TestFD, true, true);</span><br style="font-family:arial,sans-serif;font-size:12.727272033691406px">
<span style="font-family:arial,sans-serif;font-size:12.727272033691406px">>....</span><br style="font-family:arial,sans-serif;font-size:12.727272033691406px"><span style="font-family:arial,sans-serif;font-size:12.727272033691406px">>}</span><br style="font-family:arial,sans-serif;font-size:12.727272033691406px">
<span style="font-family:arial,sans-serif;font-size:12.727272033691406px">>OwningBuffer Buf;</span></div><div><br></div><div>I don't mind about buffering - really just wanted the results to be seen in the file immediately without closing it. The new patch explicitly separates the two cases: (1) file is still open and (2) file is close and then reopened for reading.</div>
<div><br><span style="font-family:arial,sans-serif;font-size:12.727272033691406px">>You probably want to pass the buff size, not the filesize down to</span><br style="font-family:arial,sans-serif;font-size:12.727272033691406px">
<span style="font-family:arial,sans-serif;font-size:12.727272033691406px">>MemoryBuffer::getOpenFile.</span><br></div><div><span style="font-family:arial,sans-serif;font-size:12.727272033691406px"><br></span></div><div>
<span style="font-family:arial,sans-serif;font-size:12.727272033691406px">You are probably right; I was following the usage of getOpenFile within gold-plugin.cpp, which deals with "files within files" and also passes the size of the sub-file into FileSize. Now looking more closely on that invocation, something doesn't make sense: When MapSize = -1, getOpenFile sets MapSize = FileSize. But then, shouldUseMmap asserts:</span></div>
<div><span style="font-family:arial,sans-serif;font-size:12.727272033691406px"><br></span></div><div><span style="font-family:arial,sans-serif;font-size:12.727272033691406px"><div> size_t End = Offset + MapSize;</div><div>
assert(End <= FileSize);</div><div><br></div><div>Meaning that if a positive offset is passed, and MapSize = 1 this assertion would fire (unless shouldUseMmap bails out for small sizes).</div><div>So is gold-plugin.cpp using the API incorrectly? Note that actually using this Offset is uncommon, ISTM only gold/lto related code uses it.</div>
<div><br></div><div>[in any case it would probably be beneficial to explain the API in more detail in a comment - I can do that with my next commit there]</div><div><br></div><div>Eli</div><div><br></div></span></div><div>
<span style="font-family:arial,sans-serif;font-size:12.727272033691406px"><br></span></div><div><span style="font-family:arial,sans-serif;font-size:12.727272033691406px"><br></span></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div><div class="h5">
On 22 July 2013 19:32, Eli Bendersky <<a href="mailto:eliben@google.com">eliben@google.com</a>> wrote:<br>
> This patch refactors the recently added unit test for<br>
> MemoryBuffer::getOpenFile to run in two different modes: with and without<br>
> reopening the temporary file between creating it and mapping it with<br>
> MemoryBuffer.<br>
><br>
> PTAL,<br>
> Eli<br>
><br>
><br>
</div></div>> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
><br>
</blockquote></div><br></div></div>