[Lldb-commits] [PATCH] D53368: [Symbol] Search symbols with name and type in a symbol file

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

Very close. Just a bit of cleanup now that we changed SymbolFilePDB where we don't seem to need m_public_symbols anymore. See inlined comments and let me know what you think



================
Comment at: include/lldb/lldb-forward.h:444
     StructuredDataPluginWP;
+typedef std::shared_ptr<lldb_private::Symbol> SymbolSP;
 typedef std::shared_ptr<lldb_private::SymbolFile> SymbolFileSP;
----------------
Do we need this anymore? See inlined comments below.


================
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:1344-1346
+    auto symbol_ptr = GetPublicSymbol(*pub_symbol);
+    if (!symbol_ptr)
+      continue;
----------------
Maybe get the file address from "pub_symbol" and avoid converting to a SymbolSP that we will never use? See more comments below.


================
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:1353
+
+    symtab.AddSymbol(*symbol_ptr);
+  }
----------------
Just make  a local symbol and convert it from PDB to lldb_private::Symbol here? Something like:
```
symtab.AddSymbol(ConvertPDBSymbol(pdb_symbol));
```

So it seems we shouldn't need the m_public_symbols storage in this class anymore since "symtab" will now own the symbol we just created. 


================
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:1968-1969
+
+lldb_private::Symbol *SymbolFilePDB::GetPublicSymbol(
+    const llvm::pdb::PDBSymbolPublicSymbol &pub_symbol) {
+  auto it = m_public_symbols.find(pub_symbol.getSymIndexId());
----------------
Maybe convert this to:
```
lldb_private::Symbol ConvertPDBSymbol(const llvm::pdb::PDBSymbolPublicSymbol &pub_symbol)
```
And we only call this if we need to create a symbol we will add to the "symtab" in SymbolFilePDB::AddSymbols(...)


================
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.h:247
   llvm::DenseMap<uint32_t, lldb::VariableSP> m_variables;
+  llvm::DenseMap<uint32_t, lldb::SymbolSP> m_public_symbols;
   llvm::DenseMap<uint64_t, std::string> m_public_names;
----------------
Do we need this mapping anymore? We should just add the symbols to the symbol table during SymbolFilePDB::AddSymbols(...).


https://reviews.llvm.org/D53368