[Lldb-commits] [PATCH] D62302: DWARF: Fix address range support in mixed 4+5 scenario

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Nice patch. See inlined comments.



================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:435-448
+          llvm::Expected<DWARFRangeList> expected_ranges =
+              (form_value.Form() == DW_FORM_rnglistx)
+                  ? cu->FindRnglistFromIndex(form_value.Unsigned())
+                  : cu->FindRnglistFromOffset(form_value.Unsigned());
+          if (expected_ranges)
+            ranges = *expected_ranges;
           else
----------------
Seems like this code should be put into a method and used here.


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:961-973
+    llvm::Expected<DWARFRangeList> expected_ranges =
+        (form_value.Form() == DW_FORM_rnglistx)
+            ? cu->FindRnglistFromIndex(form_value.Unsigned())
+            : cu->FindRnglistFromOffset(form_value.Unsigned());
+    if (expected_ranges)
+      ranges = *expected_ranges;
+    else
----------------
Seems like this code should be put into a method and used here.


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFUnit.h:208-216
+  /// Return a list of address ranges resulting from a (possibly encoded)
+  /// range list starting at a given offset in the appropriate ranges section.
+  llvm::Expected<DWARFRangeList> FindRnglistFromOffset(dw_offset_t offset) const;
+
+  /// Return a list of address ranges retrieved from an encoded range
+  /// list whose offset is found via a table lookup given an index (DWARF v5
+  /// and later).
----------------
Maybe just make one function here that takes a DWARFFormValue?:

```
llvm::Expected<DWARFRangeList> FindRnglistFromOffset(const DWARFFormValue &value) const;
```
Then it will clean up the code that calls it quite nicely.


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

https://reviews.llvm.org/D62302