[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/lib/DebugInfo/DWARF/DWARFContext.cpp:487
+                 DObj->getMacroSection().Data))
+    parseAndDumpMacroMacinfo(Macro, OS, DumpOpts, /*IsLittleEndian=*/true,
+                             dwarf::Macro);
----------------
SouraVX wrote:
> jhenderson wrote:
> > 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"?
> Are you suggesting something sort of - 
> `parseMacroOrMacinfo`  handles parsing and error handling, this can be a member function instead of a lamba for uniformities sake.
> ```
> if (shouldDump(Explicit, ".debug_macro", DIDT_ID_DebugMacro,
>                   DObj->getMacroSection().Data)) {
>      parseMacroOrMacinfo(Macro, OS, DumpOpts, /*IsLittleEndian=*/true,
>                               dwarf::Macro);
>      if (Macro)
>      Macro->dump(OS);
>    }
> ```
> I'm okay with this too :)
Most of the other sections have a pattern of "get/parse" followed by "dump", often in one line, without the error check. I think we should follow that if possible (but with the error check, or possibly by initialising the pointer to some "default" empty section in the event of a failure). In other words, yes, esssentially what you suggested. In particular, this will allow the parsing code to be used and tested independently of the printing.


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

https://reviews.llvm.org/D73086