Ping? I believe I've addressed all the comments.<div>Also (assuming approval), someone will have to commit this, since I'm not yet a committer.<br><br><div class="gmail_quote">On Thu, Dec 1, 2011 at 11:23 AM, Derek Schuff <span dir="ltr"><<a href="mailto:dschuff@google.com" target="_blank">dschuff@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>Thanks for the review, comments have been addressed</div><div>(per IRC discussion: virtual destructors left in place because moving them to BitcodeReader.cpp breaks linking clang, which includes BitstreamReader.h for AST serialization but does not depend on bitcode lib)</div>
<div><br></div><div>Patch attached.<div><div><br><br><div class="gmail_quote">On Wed, Nov 30, 2011 at 2:39 PM, Nick Lewycky <span dir="ltr"><<a href="mailto:nlewycky@google.com" target="_blank">nlewycky@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_quote"><div>On 28 November 2011 12:42, Derek Schuff <span dir="ltr"><<a href="mailto:dschuff@google.com" target="_blank">dschuff@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
thanks for the feedback, I'm fixing all the suggestions, but I do have one question:<br><br><div class="gmail_quote"><div>On Mon, Nov 21, 2011 at 3:38 PM, Nick Lewycky <span dir="ltr"><<a href="mailto:nlewycky@google.com" target="_blank">nlewycky@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>This is in a header file. Please move the implementation of the virtual destructor to BitcodeReader.cpp, or else copies will end up in every .o file that includes this header.</div>
<div><br></div><div>Same with MemoryBitstreamBytes and LazyBitstreamBytes. More general question, any reason to put MemoryBitstreamBytes and LazyBitstreamBytes implementations inside the header? You could create lib/Bitcode/Reader/BitstreamReader.cpp.</div>
<div><br></div></blockquote><div><br></div></div><div>The BitstreamBytes implementations are in the header file because BitstreamReader/BitstreamCursor is implemented entirely in the header file. Presumably BitstreamReader is in the header file so it can be inlined. Presumably this improves performance (especially in llc where there the usage is only in one file), although there seem to be several uses of BitstreamCursor in clang, so it may contribute to more code size bloat there. If you want I can move everything to BitstreamReader.cpp (perhaps LTO could still buy us back any performance loss from inlining?)</div>
</div></blockquote><div><br></div></div><div>That's a great point. Given the other code in the bitcode reader, I think it's fine to leave the implementations in the header -- except for the virtual destructors.</div>
<div><div>
<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote">
<div>It would also be possible to move just the BitstreamBytes implementations into a cpp file using the common idiom of having static factory functions with an anonymous namespace rather than just calling "new" directly like we do right now.</div>
</div></blockquote><div><br></div></div><div>I don't have a sense that either way would be particularly better.</div><div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_quote"><div>
<div>>One other thing. The behaviour when LazyBitcode is true could be really different from when it off on the same .bc file on >disk. It'd be good to add comments pointing out why (module-level assembly comes to mind, anything else?).</div>
<div><br></div></div><div>Sure. where would be a good place to put it?</div></div></blockquote><div><br></div></div><div>I was thinking of comments above the option in llc.</div><span><font color="#888888"><div>
<br></div><div>Nick</div><div><br></div></font></span></div>
</blockquote></div><br></div></div></div>
</blockquote></div><br></div>