[PATCH] D87736: [PDB] Split TypeServerSource and extend type index map lifetime

[Alexandre Ganea via Phabricator via llvm-commits]

aganea added a comment.

Just a few more things and we should be good to go:



================
Comment at: lld/COFF/PDB.cpp:191
 public:
-  DebugSHandler(PDBLinker &linker, ObjFile &file, const CVIndexMap *indexMap)
-      : linker(linker), file(file), indexMap(indexMap) {}
+  DebugSHandler(PDBLinker &linker, ObjFile &file, TpiSource *source)
+      : linker(linker), file(file), source(source) {}
----------------
Can we only have `TpiSource *` here and infer `file`?


================
Comment at: lld/COFF/PDB.cpp:276
+    ArrayRef<TypeIndex> typeOrItemMap;
+    if (source)
+      typeOrItemMap = isItemIndex ? source->ipiMap : source->tpiMap;
----------------
Don't we always expect a `source` at this point?


================
Comment at: lld/COFF/PDB.cpp:755
     TypeIndex &inlinee = *const_cast<TypeIndex *>(&line.Header->Inlinee);
-    ArrayRef<TypeIndex> typeOrItemMap =
-        indexMap->isTypeServerMap ? indexMap->ipiMap : indexMap->tpiMap;
-    if (!remapTypeIndex(inlinee, typeOrItemMap)) {
+    if (!remapTypeIndex(inlinee, source->ipiMap)) {
       log("bad inlinee line record in " + file.getName() +
----------------
`source` isn't checked here. Perhaps `assert(source)` in the `DebugSHandler` constructor?


================
Comment at: lld/COFF/PDB.cpp:921
 
-  addDebugSymbols(source->file, indexMap);
+  // If the source has an object file, add its symbol records.
+  if (source->file)
----------------
...otherwise if you don't want the `if`, it is worth adding a line to explain in which situation the source doesn't have a file.


================
Comment at: lld/COFF/PDB.cpp:923
+  if (source->file)
+    addDebugSymbols(source->file, source);
 }
----------------
No need to pass down both `file` and `source`.


================
Comment at: lld/COFF/PDB.cpp:931
-
-  if (source->kind == TpiSource::PDB)
-    return; // No symbols in TypeServer PDBs
----------------
I liked it better with this `if` as it is self-explantory. Is there another situation where we should return here? The `TypeServerIpiSource` is never returned, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87736