[PATCH] D30314: Implement some methods for NativeRawSymbol

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/PDB/Native/NativeSession.h:24-26
+  NativeSession(StringRef Path,
+                std::unique_ptr<PDBFile> PdbFile,
                 std::unique_ptr<BumpPtrAllocator> Allocator);
----------------
Is the `Path` here necessary?`  The `PDBFile` class is constructed with a Path, and you can query it via `getFilePath()`.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeRawSymbol.cpp:70
+  auto IS = File.getPDBInfoStream();
+  return IS ? IS.get().getAge() : 0;
 }
----------------
If it fails, then `IS` is going to have an `Error` in it.  And if it does, this is going to assert / crash if you don't explicitly handle the error.  Usually I do something like this:

```
if (IS)
  return IS->getAge();
return IS.takeError();
```

But in this case you have nowhere to propagate the error to, so instead you can write `consumeError(IS.takeError()); return 0;`

Granted, silently throwing away errors is not ideal, but I don't see any solution to this unless you verify that it exists when you construct the `NativeRawSymbol`.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeRawSymbol.cpp:337
+  auto &File = Session.getPDBFile();
+  auto IS = File.getPDBInfoStream();
+  return IS ? IS.get().getGuid() : PDB_UniqueId{{0}};
----------------
Same problem here.


https://reviews.llvm.org/D30314