[Lldb-commits] [PATCH] D62570: [WIP] Use LLVM's debug line parser in LLDB

[George Rimar via Phabricator via lldb-commits]

grimar added inline comments.


================
Comment at: lldb/include/lldb/Core/FileSpecList.h:29
+  typedef std::vector<FileSpec> collection;
+  typedef collection::iterator iterator;
+  typedef collection::const_iterator const_iterator;
----------------
Seems you don't use `iterator` anywhere?


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:792
+      cu_die.GetAttributeValueAsUnsigned(DW_AT_stmt_list, DW_INVALID_OFFSET);
+  if (stmt_list == DW_INVALID_OFFSET)
+    return false;
----------------
You do not use `stmt_list` anywhere else, so I would suggest to:

```
  if (cu_die.GetAttributeValueAsUnsigned(DW_AT_stmt_list, DW_INVALID_OFFSET) ==
      DW_INVALID_OFFSET)
    continue;
```


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:879
+
+  auto AddSection = [&](Section &section) {
+    DataExtractor section_data;
----------------
`AddSection`->`addSection`?


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:929-938
+  llvm::DWARFDebugLine line;
+  llvm::Expected<const llvm::DWARFDebugLine::LineTable *> line_table =
+      line.getOrParseLineTable(
+          data, offset, *ctx, ctx->getUnitForOffset(dwarf_cu->GetOffset()),
+          [](llvm::Error e) { llvm::consumeError(std::move(e)); });
+
+  if (!line_table) {
----------------
clayborg wrote:
> Can we make a function out of this that just returns the "const llvm::DWARFDebugLine::LineTable *"? Or make a new location variable that is just a "const llvm::DWARFDebugLine::LineTable *" to avoid the "(*line_table)->" stuff below?
+1 for `new location variable`.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:974
+    if (!found_original)
+      continue;
+    std::string remapped_file;
----------------
Seems you can avoid having `found_original` and `found_remapped`:
(assuming `table` is a new variable mentioned above)

```
  if (!table->getFileNameByIndex(
          index + 1, compile_dir,
          llvm::DILineInfoSpecifier::FileLineInfoKind::Default, original_file))
    continue;
```


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D62570