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.
On Tue, Mar 1, 2016 at 10:10 AM Greg Clayton <clayb...@gmail.com> wrote:
> 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
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
