[PATCH] D29973: [PDB] Support following Type server records

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/CodeView/TypeServerHandler.h:20
+
+class TypeServerHandler {
+public:
----------------
I think you want to define a virtual dtor to silence -Wnon-virtual-dtor.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/PDBTypeServerHandler.cpp:7
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
----------------
Can you write a file comment what we are doing in this file? E.g. a LF_TYPESERVER2 record referes an external pdb file, so we read that file and replace TypeServer2Record with the file contents.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/PDBTypeServerHandler.cpp:33
+
+void PDBTypeServerHandler::addSearchPath(StringRef Path) {
+  if (Path.empty() || !sys::fs::is_directory(Path))
----------------
Calling this function more than once with the same argument seems to add the same element to to `SearchPaths`. Is this a desired behavior?


================
Comment at: llvm/lib/DebugInfo/PDB/Native/PDBTypeServerHandler.cpp:40
+
+Expected<bool>
+PDBTypeServerHandler::handleInternal(PDBFile &File,
----------------
You want to change this to `Error` and return `Error::success()` on success.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/PDBTypeServerHandler.cpp:80
+      // doesn't match and we should continue searching.
+      ignoreErrors(std::move(EC));
+      continue;
----------------
I believe that evaluating EC as a boolean value clears "checked" bits of the object, so you can safely ignore errors without calling this.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/PDBTypeServerHandler.cpp:92
+
+    ArrayRef<uint8_t> GuidBytes(ExpectedInfo->getGuid().Guid);
+    StringRef GuidStr(reinterpret_cast<const char *>(GuidBytes.begin()),
----------------
Please add a comment -- GUID must be the same.


https://reviews.llvm.org/D29973