[PATCH] D31838: Bitcode: Add a string table to the bitcode format.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc marked 6 inline comments as done.
pcc added inline comments.


================
Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:1285
 
-  // Emit the module's source file name.
-  {
----------------
tejohnson wrote:
> Why does this move?
We now create the summary entries as we read `MODULE_CODE_*` records. That means that we need the source file name to compute the GUID correctly.


================
Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:1310
-
-  // If we have a VST, write the VSTOFFSET record placeholder.
-  if (M.getValueSymbolTable().empty())
----------------
tejohnson wrote:
> Why does this go away?
We now create a VST unconditionally. Avoiding creating a VST if the module is empty seems like a micro-optimization that is not worth it.


================
Comment at: llvm/tools/llvm-cat/llvm-cat.cpp:54
   } else {
+    std::vector<std::unique_ptr<Module>> OwnedMods;
     for (std::string InputFilename : InputFilenames) {
----------------
tejohnson wrote:
> Needed because string in string table owned by parsed modules?
Yes, comment added.


https://reviews.llvm.org/D31838