[PATCH] D31921: Object: Factor out the code for creating the irsymtab for an arbitrary bitcode file.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: llvm/include/llvm/Object/IRObjectFile.h:67
+
+namespace irsymtab {
+
----------------
tejohnson wrote:
> why have this in the irsymtab namespace and not object? My main motivation for the move here is that irsymtab doesn't seem like the right namespace/layer for having the main entry point reading/building the full bitcode file contents. Or a new namespace (e.g. bitcodefile or bitcodeobject or the like).
To me these are still conceptually part of the irsymtab, I moved them here at your request. But I'm not going to pick this particular battle either, so ok, let's move them to `llvm::object`. Does `llvm::object::IRSymtabFile`/`llvm::object::readIRSymtab` sound fine?


================
Comment at: llvm/include/llvm/Object/IRSymtab.h:322
+/// for the irsymtab are owned by Symtab and Strtab.
+struct FileContents {
+  SmallVector<char, 0> Symtab, Strtab;
----------------
tejohnson wrote:
> Maybe IRSymtabContents? I.e. this doesn't contain the full contents of the bitcode file.
It is already in a namespace called `irsymtab`. There's no need to repeat that in the name here.

That said, I should update the comment to reflect that this just contains the irsymtab contents.


================
Comment at: llvm/include/llvm/Object/IRSymtab.h:328
+/// Reads the contents of a bitcode file, creating its irsymtab if necessary.
+Expected<FileContents> readBitcode(ArrayRef<BitcodeModule> Mods);
+
----------------
tejohnson wrote:
> maybe readIRSymtab?
Same.


https://reviews.llvm.org/D31921