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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
