[PATCH] D27077: LTO: Port the new LTO API to ModuleSymbolTable.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: llvm/include/llvm/LTO/LTO.h:104
   friend LTO;
-  InputFile() = default;
+  InputFile(MemoryBufferRef MBRef) : MBRef(MBRef) {}
 
----------------
mehdi_amini wrote:
> Why don't you take all the members here?
> It seems strange to see the initialization and then `File->Mod = ...`
We need to create the module using the LLVMContext owned by this class.

I suppose that given that we need to do that we might as well be consistent with the other members and let MBRef be default initialized as well.


================
Comment at: llvm/include/llvm/LTO/LTO.h:166
+           const ModuleSymbolTable *SymTab, const InputFile *File)
+        : I(I), SymTab(SymTab), File(File) {
       skip();
----------------
mehdi_amini wrote:
> Can you turn SymTab and File to be reference?
Done for SymTab. File may be null (see addRegularLTO) so I've left it as a pointer.


================
Comment at: llvm/include/llvm/LTO/LTO.h:171
     StringRef getName() const { return Name; }
     StringRef getIRName() const {
+      if (isGV())
----------------
mehdi_amini wrote:
> Non relevant for this patch, but it'd be nice to document getName vs getIRName, I always forget when I look at this.
It looks like getIRName is only used by the implementation, so I've removed it in r288302 and added documentation for getName.


================
Comment at: llvm/include/llvm/LTO/LTO.h:248
 
-  StringRef getSourceFileName() const {
-    return Obj->getModule().getSourceFileName();
-  }
-
-  MemoryBufferRef getMemoryBufferRef() const {
-    return Obj->getMemoryBufferRef();
-  }
+  StringRef getSourceFileName() const { return Mod->getSourceFileName(); }
+  MemoryBufferRef getMemoryBufferRef() const { return MBRef; }
----------------
mehdi_amini wrote:
> Looks like something to add to `ModuleSymbolTable`.
> (I don't mean it as a blocker for this patch)
Maybe not, given our discussion on D27073.


https://reviews.llvm.org/D27077