[PATCH] D45549: [DWARF v5] improved support for .debug_rnglists/consumer

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

This looks generally good to me.



================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugRnglists.h:120
+    default:
+      llvm_unreachable("Invalid DWARF format (expected DWARF32 or DWARF64");
+    }
----------------
JDevlieghere wrote:
> Is `Format` checked before this function is called? If not, this should probably also be an `Optional` as it can fail due to bad input. Otherwise I'd add a comment specifying this as a pre-condition. 
Nit: By moving the unreachable outside of the switch statement, we could benefit from the incomplete-switch warning if a new enumerator is added.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:255
+    if (Error Err = Rnglists.extract(rnglistData, &Offset)) {
+      errs() << "error: " + toString(std::move(Err)) << '\n';
+      uint64_t Length = Rnglists.length();
----------------
Not your code, but since you touched it: IIRC there's supposed to be a helper function that prints "error:" in color somewhere in libDebugInfo.


================
Comment at: lib/DebugInfo/DWARF/DWARFUnit.cpp:381
+DWARFAddressRangesVector DWARFUnit::findRnglistFromOffset(uint32_t Offset) {
+  if (getVersion() >= 5) {
+    if (RngListTable) {
----------------
That's a matter of taste, but I would have put the Version <= 4 case up front since its shorter.


https://reviews.llvm.org/D45549