[PATCH] D35504: [PDB] Merge in types and items from type servers (/Zi)

[Reid Kleckner via Phabricator via llvm-commits]

rnk marked an inline comment as done.
rnk added inline comments.


================
Comment at: lld/COFF/PDB.cpp:125
+  /// Type index mappings of type server PDBs that we've loaded so far.
+  std::map<WindowsGuid, CVIndexMap> TypeServerIndexMappings;
 };
----------------
ruiu wrote:
> Do you need an ordered map?
I don't need ordering, but the keys and values are large (16 byte GUID + six pointers), so I want a container with a separate allocation. std::unordered_map is probably appropriate, but DenseMap is not.


================
Comment at: lld/COFF/PDB.cpp:223-226
+  auto ExpectedInfo = File.getPDBInfoStream();
+  // All PDB Files should have an Info stream.
+  if (!ExpectedInfo)
+    return ExpectedInfo.takeError();
----------------
ruiu wrote:
> Is this correct that you  are calling `getPDBInfoStream` only to check the existence of an Info stream?
We need it for the Guid checking code below.


================
Comment at: lld/COFF/PDB.cpp:230-238
+  bool CheckGuids = false;
+  if (CheckGuids) {
+    // Just because a file with a matching name was found and it was an actual
+    // PDB file doesn't mean it matches.  For it to match the InfoStream's GUID
+    // must match the GUID specified in the TypeServer2 record.
+    if (ExpectedInfo->getGuid() != GuidFromObj)
+      return make_error<pdb::GenericError>(
----------------
ruiu wrote:
> Can you remove this code for now?
Oh, the whole point of rebasing this on D35495 was to remove this check. We can check the GUIDs now that YAML serialization works.


https://reviews.llvm.org/D35504