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

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFUnit.h:148
   dw_addr_t GetStrOffsetsBase() const { return m_str_offsets_base; }
+  dw_offset_t GetLineTableOffset();
   void SetAddrBase(dw_addr_t addr_base);
----------------
Nit: move this next to `GetAbbrevOffset`


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFUnit.h:200
   const lldb_private::FileSpec &GetCompilationDirectory();
+  const lldb_private::FileSpec &GetFileSpec();
   lldb_private::FileSpec::Style GetPathStyle();
----------------
Given the implementation, how about `GetAbsolutePath`? I think the "FileSpec" of a DWARF unit is a little weird, but feel free to ignore this. 


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFUnit.h:290
   dw_offset_t m_str_offsets_base = 0; // Value of DW_AT_str_offsets_base.
+  dw_offset_t m_line_table_offset =
+      DW_INVALID_OFFSET; // Value of DW_AT_stmt_list.
----------------
Let's make this a Doxygen comment with `///<` or even better just `///` on the line above it so it doesn't wrap. 


================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:821
+  if (iter_bool.second) {
+    list.Append(FileSpec());
+    DWARFDebugLine::ParseSupportFiles(GetObjectFile()->GetModule(),
----------------
Not your responsibility but we should really get rid of this thing as it doesn't make sense anymore in > DWARF5.


================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h:303
 
+  lldb_private::FileSpec GetSupportFile(DWARFUnit &unit, size_t file_idx);
+
----------------
clayborg wrote:
> Can probably just be named GetFile
Should this return an optional?


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

https://reviews.llvm.org/D62894