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

[Greg Clayton via Phabricator via llvm-commits]

clayborg added inline comments.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:289
+  bool success = true;
+  if (DumpType == DIDT_All || DumpType == DIDT_Info) {
+    OS << "Verifying .debug_info...\n";
----------------
probinson wrote:
> 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.
I am breaking my change up into many piece more to come. This if should remain for now since the a future patch will need this to be here. 


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:301
+          const auto Form = AttrValue.Value.getForm();
+          switch (Attr) {
+            case DW_AT_ranges:
----------------
probinson wrote:
> 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.
I can add a comment. 


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:391
+            }
+            default:
+              break;
----------------
probinson wrote:
> 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.
We would mention them here, but I think that might clutter things. 


https://reviews.llvm.org/D32707