[PATCH] D35738: Enable llvm-pdbutil to list enumerations using native PDB reader

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeEnumSymbol.cpp:17-18
+
+namespace llvm {
+namespace pdb {
+
----------------
Can you do

```
using namespace llvm;
using namespace llvm::pdb;
```

instead of putting the namespace inline?  That's the preferred approach for LLVM style.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeEnumSymbol.cpp:21
+NativeEnumSymbol::NativeEnumSymbol(NativeSession &Session, SymIndexId Id,
+                                   const codeview::CVType &fooCV)
+    : NativeRawSymbol(Session, Id), CV(fooCV),
----------------
Can you choose a better name than `fooCV`?  :)


================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeEnumSymbol.cpp:24
+      Record(codeview::TypeRecordKind::Enum) {
+  cantFail(visitTypeRecord(CV, *this));
+}
----------------
should we assert that `fooCV.type() == TypeLeafKind::LV_ENUM`?  Ideally it would be nice if this constructor were private and there was a single method that would create a Native symbol from a `CVType` that would create the right concrete type.  But we can save that for later.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeSession.cpp:113
+  auto Tpi = Pdb->getPDBTpiStream();
+  if (Tpi) {
+    auto &Types = Tpi->typeCollection();
----------------
early return?


================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeSession.cpp:118
+    const auto Id = static_cast<SymIndexId>(SymbolCache.size());
+    SymbolCache.push_back(llvm::make_unique<NativeEnumSymbol>(*this, Id, I));
+    return llvm::make_unique<PDBSymbolTypeEnum>(
----------------
It looks like this will create a new instance every time someone calls this method?  I thought it was going to do a scan up front and cache everything, and then subsequent calls would just return directly from the cache?


https://reviews.llvm.org/D35738