<div dir="ltr">Hi Karl,<br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, May 28, 2015 at 3:09 PM, Karl Schimpf <span dir="ltr"><<a href="mailto:kschimpf@google.com" target="_blank">kschimpf@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Fixes based on feedback by Filipe.<br>
<span class=""><br>
<br>
================<br>
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:289<br>
@@ -251,1 +288,3 @@<br>
+ /// \param M the module to build.<br>
<br>
+ /// \param ShouldMaterializeAll true when the module should be materialized<br>
----------------<br>
</span>filcab wrote:<br>
> Why the empty line?<br>
Removed.<br>
<span class=""><br>
================<br>
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:789<br>
@@ -717,3 +788,3 @@<br>
namespace {<br>
- /// @brief A class for maintaining the slot number definition<br>
+ /// \brief A class for maintaining the slot number definition<br>
/// as a placeholder for the actual definition for forward constants defs.<br>
----------------<br>
</span>filcab wrote:<br>
> Omit \brief.<br>
Done.<br>
<span class=""><br>
================<br>
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:803<br>
@@ -731,3 +802,3 @@<br>
<br>
- /// @brief Methods to support type inquiry through isa, cast, and dyn_cast.<br>
+ /// \brief Methods to support type inquiry through isa, cast, and dyn_cast.<br>
static bool classof(const Value *V) {<br>
----------------<br>
</span>filcab wrote:<br>
> Omit \brief.<br>
Done.<br>
<span class=""><br>
================<br>
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:3254<br>
@@ -3125,2 +3253,3 @@<br>
case BitstreamEntry::EndBlock:<br>
+ setParseState(AtTopLevel);<br>
return std::error_code();<br>
----------------<br>
</span><span class="">filcab wrote:<br>
> We're already at top level, no? (line 3234)<br>
> I might be missing something, but it looks like we're at top level, and saw an EndBlock. Shouldn't this be an error?<br>
</span>I agree that we should be at top level.<br>
<br>
I also agree that it appears weird that we allow extra (unmathced) EndBlocks. This has been allowed by the bitcode reader/writer for years. I just wasn't willing to make the leap that I should remove this.<br>
<br>
However, I tried removing it (and making it an error), and no tests failed. Hence, Converting this to an error.<br>
<br>
================<br>
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:4676<br>
@@ -4517,2 +4675,3 @@<br>
+ return EC;<br>
<span class=""><br>
// Iterate over the module, deserializing any functions that are still on<br>
</span>----------------<br>
Moved the iterating of functions to before the call to finishParse. This deals with the problems I was having with tests in test/Bitcode/invalid.test (i.e. Inputs/invalid-fwdref-type-mismatch-2.bc and Inputs/invalid-load-ptr-type.bc). These two files had multiple errors (the one they intended which was inside a function body, and the one probably not intended - extraneous stuff at the end of the bitcode file).<br>
<br>
This removes the need for the command line flag UseOldLazyBitcodeParse, and I have deleted it.<br></blockquote><div><br></div><div>The files came straight from the fuzzer, so it is likely have more than one error. If, in order to support them (where support is: keep the test working and diagnosing what we want), you have to change the code in a convoluted way, I would prefer to change the test.</div><div><br></div><div>If the change is minimal and not a problem (doesn't impact legibility or architecture), then keeping the tests as they are is not a problem either. I just want to avoid having worse code just so we don't have to re-do some tests.</div><div><br></div><div>Of course, if we started crashing on the tests, that's a problem :-)</div><div><br></div><div>Thanks,</div><div><br></div><div> Filipe</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><br>
<a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D8786&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=zTYhj_49AXXhfnx-UpGQnVGeOWnBUD5VAsrtsTOJth4&s=fvEP2fMLuYuyFT9yzEweP9ol71T3jdB4SImzVFT3FR8&e=" target="_blank">http://reviews.llvm.org/D8786</a><br>
<br>
EMAIL PREFERENCES<br>
<a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_settings_panel_emailpreferences_&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=zTYhj_49AXXhfnx-UpGQnVGeOWnBUD5VAsrtsTOJth4&s=MBWNnYnB8GfozS1GOh4w1oUU50gkqYy4SN_gDbfIlvs&e=" target="_blank">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
<br>
<br>
</div></div></blockquote></div><br></div></div>