clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
One general comment is the use of "auto". Although it makes the code shorter,
it does make it quite a bit less readable. I will leave the decision to you
since this is your code, but in general I think this is where auto is less than
it is cracked up to be.
================
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:282-285
@@ +281,6 @@
+
+uint32_t
+SymbolFilePDB::ResolveSymbolContext(const lldb_private::FileSpec &file_spec,
uint32_t line, bool check_inlines,
+ uint32_t resolve_scope,
lldb_private::SymbolContextList &sc_list)
+{
+ if (resolve_scope & lldb::eSymbolContextCompUnit)
----------------
So looking at SymbolFileDWARF::ResolveSymbolContext() I see it is very close to
being SymbolFile agnostic... We would make
SymbolFileDWARF::ResolveSymbolContext() into SymbolFile::ResolveSymbolContext()
and clean it up to just use virtual SymbolFile calls. Then all SymbolFile
plug-ins wouldn't need to implement this function. The basic flow of the
function in DWARF is to iterate through all compile units. If check_inlines is
true or the compile unit matches, grab the support files via
lldb_private::CompileUnit::GetSupportFiles() and see if "file_spec" is in that
support files list and find the one and only index for that file. If the index
is valid, then get the LineTable from the compile unit via
lldb_private::CompileUnit::GetLineTable(). Then find all matching entries. So
with a quick refactor, all we need new SymbolFile instances to implement is
GetSupportFiles() (which calls SymbolFile::ParseCompileUnitSupportFiles()) and
CompileUnit::GetLineTable() (which calls into
SymbolFile::ParseCompileUnitLineTable()). What do you think? This also helps
others implementing new SymbolFile classes to not have to worry about the
check_inlines thing.
================
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:291-292
@@ +290,4 @@
+ // <vector>, either directly or indirectly.
+ auto compilands =
+ m_session_up->findCompilandsForSourceFile(file_spec.GetPath(),
llvm::PDB_NameSearchFlags::NS_CaseInsensitive);
+
----------------
So if file_spec is "vector", this function will return all compile units that
have line table entries that match "vector"? It doesn't seem like this is
correct. If "check_inlines" is true, I would expect that you need to traverse
all compilands?
================
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:428-448
@@ +427,23 @@
+{
+ auto found_cu = m_comp_units.find(id);
+ if (found_cu != m_comp_units.end())
+ return found_cu->second;
+
+ auto cu =
m_session_up->getConcreteSymbolById<llvm::PDBSymbolCompiland>(id);
+
+ // `getSourceFileName` returns the basename of the original source file
used to generate this compiland. It does
+ // not return the full path. Currently the only way to get that is to do
a basename lookup to get the
+ // IPDBSourceFile, but this is ambiguous in the case of two source files
with the same name contributing to the
+ // same compiland. This is a moderately extreme edge case, so we consider
this ok for now, although we need to find
+ // a long term solution.
+ auto file = m_session_up->findOneSourceFile(cu.get(),
cu->getSourceFileName(),
+
llvm::PDB_NameSearchFlags::NS_CaseInsensitive);
+ std::string path = file->getFileName();
+
+ lldb::LanguageType lang;
+ auto details = cu->findOneChild<llvm::PDBSymbolCompilandDetails>();
+ if (!details)
+ lang = lldb::eLanguageTypeC_plus_plus;
+ else
+ lang = TranslateLanguage(details->getLanguage());
+
----------------
"auto" really makes it hard to read this code to figure out what each variable
actually is from someone that doesn't know the code. I will leave it up to you
to do what you will with this, but this is where auto falls down for me.
http://reviews.llvm.org/D17363
_______________________________________________
lldb-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
