[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/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;
----------------
zturner wrote:
> 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.
Alternatively, maybe we don't even need a cache?  `LazyRandomTypeCollection` already has O(1) amortized random access, as it is effectively already the cache.  So maybe you can create everything on the fly, and not even have a cache?


https://reviews.llvm.org/D35163