thanks for the feedback, I'm fixing all the suggestions, but I do have one question:<br><br><div class="gmail_quote">On Mon, Nov 21, 2011 at 3:38 PM, Nick Lewycky <span dir="ltr"><<a href="mailto:nlewycky@google.com">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>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>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><br></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>Sure. where would be a good place to put it?</div><div><br></div><div>thanks,</div><div>-Derek</div><div><br></div></div>