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

[Adrian McCarthy via Phabricator via llvm-commits]

amccarth marked 6 inline comments as done.
amccarth added inline comments.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeEnumSymbol.cpp:17-18
+
+namespace llvm {
+namespace pdb {
+
----------------
zturner wrote:
> Can you do
> 
> ```
> using namespace llvm;
> using namespace llvm::pdb;
> ```
> 
> instead of putting the namespace inline?  That's the preferred approach for LLVM style.
Um. OK, but that seems an odd style in general, since it would be prone to name collisions.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeEnumSymbol.cpp:24
+      Record(codeview::TypeRecordKind::Enum) {
+  cantFail(visitTypeRecord(CV, *this));
+}
----------------
zturner wrote:
> 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.
Sure.

Now they are constructed only by NativeSession, which has such an assertion.  I guess all these NativeXXXSymbol classes should have private constructors, and the NativeSession could be a friend to them all.


================
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>(
----------------
zturner wrote:
> 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?
The NativeEnumTypes "iterator" caches just the TypeIndexes and lazily calls this method to create the actual symbols when they're needed.

I think you're right that there's a hole in the logic here that could cause it to create the same symbol multiple times, which is a problem.  I'll fix that by moving this to findSymbolByTypeIndex.  (See the TODO below.)


https://reviews.llvm.org/D35738