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

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: llvm/tools/llvm-modextract/llvm-modextract.cpp:67
+    Writer.copyStrtab(Ms[ModuleIndex].getStrtab());
+    Out->os() << Header;
     Out->keep();
----------------
vsk wrote:
> I see two places where we need to perform a byte-for-byte copy of a BitcodeModule's buffer, and then copy the strtab. Is there any reason to perform such a copy and not copy the strtab? If not, we could reduce the duplication by adding a 'writeCopyOfModule(BitcodeModule &)' helper to BitcodeWriter.
> Is there any reason to perform such a copy and not copy the strtab?
In principle you could concatenate multi-module bitcode files by copying only unique strtabs instead of potentially copying the same strtab multiple times.

> If not, we could reduce the duplication by adding a 'writeCopyOfModule(BitcodeModule &)' helper to BitcodeWriter.
Maybe, but we only need to perform these copies in low level tools, so I'm not sure if we want to support their operations at the API level. That would also create a direct dependency from the bitcode writer to the bitcode reader, and I'd like to avoid adding any new such dependencies. Maybe we can reconsider if we need to do this elsewhere (i.e. rule of 3).


https://reviews.llvm.org/D31838