<div style="white-space:pre-wrap">Regarding the refactoring of ResolveSymbolContext to a lower level. It seems like a worthwhile refactor but probably one that should be done as an independent CL. It seems like it has potential to open up a bit of a rats nest so to speak, and if something ends up breaking as a result, we can revert just the targeted refactor instead of the entirety of PDB support.</div><br><div class="gmail_quote"><div dir="ltr">On Tue, Mar 1, 2016 at 10:10 AM Greg Clayton <<a href="mailto:clayborg@gmail.com">clayborg@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">clayborg requested changes to this revision.<br>
clayborg added a comment.<br>
This revision now requires changes to proceed.<br>
<br>
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.<br>
<br>
<br>
================<br>
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:282-285<br>
@@ +281,6 @@<br>
+<br>
+uint32_t<br>
+SymbolFilePDB::ResolveSymbolContext(const lldb_private::FileSpec &file_spec, uint32_t line, bool check_inlines,<br>
+ uint32_t resolve_scope, lldb_private::SymbolContextList &sc_list)<br>
+{<br>
+ if (resolve_scope & lldb::eSymbolContextCompUnit)<br>
----------------<br>
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.<br>
<br>
================<br>
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:291-292<br>
@@ +290,4 @@<br>
+ // <vector>, either directly or indirectly.<br>
+ auto compilands =<br>
+ m_session_up->findCompilandsForSourceFile(file_spec.GetPath(), llvm::PDB_NameSearchFlags::NS_CaseInsensitive);<br>
+<br>
----------------<br>
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?<br>
<br>
================<br>
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:428-448<br>
@@ +427,23 @@<br>
+{<br>
+ auto found_cu = m_comp_units.find(id);<br>
+ if (found_cu != m_comp_units.end())<br>
+ return found_cu->second;<br>
+<br>
+ auto cu = m_session_up->getConcreteSymbolById<llvm::PDBSymbolCompiland>(id);<br>
+<br>
+ // `getSourceFileName` returns the basename of the original source file used to generate this compiland. It does<br>
+ // not return the full path. Currently the only way to get that is to do a basename lookup to get the<br>
+ // IPDBSourceFile, but this is ambiguous in the case of two source files with the same name contributing to the<br>
+ // same compiland. This is a moderately extreme edge case, so we consider this ok for now, although we need to find<br>
+ // a long term solution.<br>
+ auto file = m_session_up->findOneSourceFile(cu.get(), cu->getSourceFileName(),<br>
+ llvm::PDB_NameSearchFlags::NS_CaseInsensitive);<br>
+ std::string path = file->getFileName();<br>
+<br>
+ lldb::LanguageType lang;<br>
+ auto details = cu->findOneChild<llvm::PDBSymbolCompilandDetails>();<br>
+ if (!details)<br>
+ lang = lldb::eLanguageTypeC_plus_plus;<br>
+ else<br>
+ lang = TranslateLanguage(details->getLanguage());<br>
+<br>
----------------<br>
"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.<br>
<br>
<br>
<a href="http://reviews.llvm.org/D17363" rel="noreferrer" target="_blank">http://reviews.llvm.org/D17363</a><br>
<br>
<br>
<br>
</blockquote></div>