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

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/PDB/Native/NativeEnumTypes.h:22
+
+class NativeEnumTypes : public IPDBEnumChildren<PDBSymbol> {
+public:
----------------
I'm wondering if we can get away with implementing these findChildren methods by building a vector of PDBSymbols up front, and then this class becomes a simple adapter from vector to IPDBEnumChildren. It seems to me if the caller calls findChildren, they're going to walk them all, so we can do O(n) work without slowing things down asymptotically.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeExeSymbol.cpp:48-49
+      const auto &Types = Tpi->types(&HadError);
+      if (HadError)
+        return nullptr;
+      return std::unique_ptr<IPDBEnumSymbols>(
----------------
This check will never fail. The optional HadError parameter is essentially an awkward way to thread error handling out of a range-based-for. The idea is that if the iterator encounters an error, it halts iteration and sets *HadError to true. The caller can check for failure outside the loop. We don't want to eagerly check for errors because that would require iterating over all the data.

Does this method get called often or early? I'm wondering if it would be better to iterate and filter the type stream up front and make a vector.


https://reviews.llvm.org/D35738