[PATCH] D51081: [DWARF v5] Refactoring range list dumping to fold DWARF v4 functionality into v5 handling (almost NFC).

[Victor Leschuk via Phabricator via llvm-commits]

vleschuk added inline comments.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugRnglists.h:67
+  getAbsoluteRanges(llvm::Optional<BaseAddress> BaseAddr,
+                    uint16_t Version) const;
 };
----------------
I suggest that DWARFListType::extract() saves Version in class data, and thus we don't need to specify it here explicitly. Same suggestion regarding RangeListEntry and other classes.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFListTable.h:68
+  void dump(raw_ostream &OS, DWARFContext *C, uint8_t AddressSize,
+            uint64_t BaseAddress, unsigned Indent, uint16_t Version,
+            size_t MaxEncodingStringLength = 0,
----------------
Why does dump() method need Version argument? We set the version when we are extracting the data. Maybe save it somewhere during extraction?


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFListTable.h:240
+  uint16_t getVersion() const { return Header.getVersion(); }
+  uint32_t length() const { return Header.length(); }
 };
----------------
To make naming consistent maybe we should rename it to getLength()?


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugRnglists.cpp:86
+        return createStringError(
+            errc::invalid_argument,
+            "invalid range list entry at offset 0x%" PRIx32, *OffsetPtr);
----------------
Maybe illegal_byte_sequence would be more suitable here?


https://reviews.llvm.org/D51081