[PATCH] D35163: [PDB] Enable NativeSession to create symbols for built-in types on demand

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/PDB/Native/NativeSession.h:13
 
+#include <unordered_map>
+
----------------
Can you use `llvm::DenseMap` instead (and delete this `#include`)


================
Comment at: llvm/include/llvm/DebugInfo/PDB/Native/NativeSession.h:42
 
+  uint32_t findSymbolByTypeIndex(uint32_t TI);
+
----------------
rnk wrote:
> Any reason not to use codeview::TypeIndex for this parameter? Just trying to avoid exposing internal libraries out through this interface?
+1


================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeSession.cpp:107-108
+  // First see if it's already in our cache.
+  const auto Entry = TypeIndexToSymbolId.find(Index);
+  if (Entry != TypeIndexToSymbolId.end())
+    return Entry->second;
----------------
I can think of a couple of issues with this function.

1. I might be misunderstanding the purpose of `findSymbolByTypeIndex`, but doesn't `TypeIndex` imply that this function *only* works for types?  What about symbols, which don't have a type index?  Would those get a different function?

2. What about id types (e.g. types from the IPI string instead of the TPI stream)?  Would we eventually add a `findSymbolByIdIndex` too?

3. Why even have a map?  Why not just have a `std::vector<std::unique_ptr<NativeRawSymbol>> Types;` and then in the constructor you resize it to `Tpi.getNumTypeRecords()`, and in this function if it's simple you create one on the fly, otherwise you access `Types[TI.toArrayIndex()]`, and if it's `nullptr` you create a new one otherwise you return the existing one?  This would be O(1) instead of doing the hash computation.


https://reviews.llvm.org/D35163