[Lldb-commits] [PATCH] D62894: DWARF: Share line tables of type units

[Pavel Labath via Phabricator via lldb-commits]

labath marked 11 inline comments as done.
labath added inline comments.


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:309
+      decl.SetFile(
+          die.GetDWARF()->GetFile(*die.GetCU(), form_value.Unsigned()));
       break;
----------------
clayborg wrote:
> At the very least we should be asking the unit for the file? That is why I wanted to be able to ask the DWARFDIE for the file because it contains the right unit. If we just put the API on the unit, then we have the chance someone will use the wrong unit for the file. Also, as the DWARF gets fancier as time goes on (DWO, DWZ, etc), the unit might refer to another unit. But at the very least here I would feel better if we ask the DWARFUnit for the file. The GetDWARF() will ask the DWARFUnit in the DIE for the is DWARF file, then we will call the DWARF file class (SymbolFileDWARF or DWARFContext to get a file from the unit by passing a reference? Seems convoluted. Fine not adding the API as a FileSpec if we are trying to keep the API the same as LLVM.
I think asking the unit for the file is fine. I'm not sure why llvm does not do that (it does the "get the context, pass the unit to get the line table, fetch the file" dance), but it sounds like a utility function doing this could be added there. Eventually our DWARFUnit will start using DWARFContext instead of SymbolFileDWARF for the other operations, which will make the two approaches pretty similar.

I've kept the function as returning FileSpecs. Returning any other type would be a pessimization, as the values are already parsed as FileSpecs.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62894/new/

https://reviews.llvm.org/D62894