[PATCH] D54802: [LLD][COFF] Generate import modules in PDB

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: lld/trunk/COFF/PDB.cpp:1111
+    SC.Characteristics = OS ? OS->Header.Characteristics : 0;
     // FIXME: When we start creating DBI for import libraries, use those here.
     SC.Imod = Modi;
----------------
Is this relevant to this change?


================
Comment at: lld/trunk/COFF/PDB.cpp:1566
+
+  std::map<std::string, llvm::pdb::DbiModuleDescriptorBuilder *> ModSet;
+
----------------
DllModSet, maybe? DllToModuleDbi?


================
Comment at: lld/trunk/COFF/PDB.cpp:1584-1586
+        SmallString<128> Name;
+        S.toVector(Name);
+        auto *M = &ExitOnErr(DbiBuilder.addModuleInfo(Name));
----------------
Usually we use Twine to avoid the cost of concatenating strings used for debugging purposes, like names for instructions that aren't used in release mode, or log statements. In this case, I think you can use plain old std::string, something like `addModuleInfo(std::string("Import:") + File->DLLName`. With that simplification, I think the lambda is unnecessary.


================
Comment at: llvm/trunk/lib/DebugInfo/PDB/Native/ModuleDebugStream.cpp:40-50
+  if (Mod.getModuleStreamIndex() != llvm::pdb::kInvalidStreamIndex) {
+    if (Error E = reloadSerialize(Reader))
+      return E;
+  }
+  if (Reader.bytesRemaining() > 0)
+    return make_error<RawError>(raw_error_code::corrupt_file,
+                                "Unexpected bytes in module stream.");
----------------
I don't see the change to add this method to the class in this patch. Was this change intended to be part of this review? It seems unnecessary for import files.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54802/new/

https://reviews.llvm.org/D54802