[PATCH] D73086: [DWARF5] Added support for debug_macro section parsing and dumping in llvm-dwarfdump.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h:276
 
-  /// Get a pointer to the parsed DebugMacro object.
-  const DWARFDebugMacro *getDebugMacinfo();
-
-  /// Get a pointer to the parsed dwo DebugMacro object.
-  const DWARFDebugMacro *getDebugMacinfoDWO();
-
+  /// Parse and Dump macro[.dwo] and macinfo[.dwo] section.
+  void parseAndDumpMacroMacinfo(std::unique_ptr<DWARFDebugMacro> &MacroPtr,
----------------
Dump -> dump a

and macinfo -> or macinfo


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:327-329
+    // FIXME: Add support for debug_macro.dwo section.
+  case dwarf::MacroDWO:
+    break;
----------------
Comment indentation is slightly off. Might be worth reporting a warning in the case that this is hit?


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:487
+                 DObj->getMacroSection().Data))
+    parseAndDumpMacroMacinfo(Macro, OS, DumpOpts, /*IsLittleEndian=*/true,
+                             dwarf::Macro);
----------------
I'm not particularly keen that dumping and parsing happen in the same function. It feels like those are two separate tasks that should be kept independent. Perhaps a small lambda near here instead would make sense to factor out the sequence "parse -> check not null -> dump"?


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

https://reviews.llvm.org/D73086