[PATCH] D32707: lldb-dwarfdump -verify [-quiet]

[Paul Robinson via Phabricator via llvm-commits]

probinson added a comment.

Mostly formatting and commentary points.  I haven't looked at the test yet.



================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:289
+  bool success = true;
+  if (DumpType == DIDT_All || DumpType == DIDT_Info) {
+    OS << "Verifying .debug_info...\n";
----------------
I know this is the idiom used everywhere else, but here instead of the 'if' guarding a call or a just a few lines, it's guarding essentially the entire method.  I'd rather see this inverted to an early exit here, and reduce indentation in the rest of the method.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:301
+          const auto Form = AttrValue.Value.getForm();
+          switch (Attr) {
+            case DW_AT_ranges:
----------------
So, most cross-section references are verified based on the attribute (because they use forms that can refer to different sections based on context), but sometimes they are just based on the form (because the form only allows a reference to one other section, like FORM_strp).  Did I get that right?  It would be nice to see a comment alluding to why some references are done one way and some another way.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:302
+          switch (Attr) {
+            case DW_AT_ranges:
+              // Make sure the offset in the DW_AT_ranges attribute is valid.
----------------
This indentation doesn't look right; maybe use clang-format?


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:391
+            }
+            default:
+              break;
----------------
I wonder if forms that should have been handled by-attribute should be explicitly documented here.  Or maybe that would be too much clutter?  I am not sure.


https://reviews.llvm.org/D32707