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

[Sourabh Singh Tomar via Phabricator via llvm-commits]

SouraVX marked 2 inline comments as done.
SouraVX added inline comments.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:327-329
+    // FIXME: Add support for debug_macro.dwo section.
+  case dwarf::MacroDWO:
+    break;
----------------
jhenderson wrote:
> jhenderson wrote:
> > SouraVX wrote:
> > > jhenderson wrote:
> > > > Comment indentation is slightly off. Might be worth reporting a warning in the case that this is hit?
> > > Before revising this patch, I want to have your opinion on this -
> > > I'm considering removing this case `MacroDWO` instead of emitting a warning(I'm not against it). But since that part of code case(MacroDWO) will never be hit, since we don't do it explicitly in first place here --
> > > ```
> > > if (shouldDump(Explicit, ".debug_macro.dwo", DIDT_ID_DebugMacro,
> > >                  DObj->getMacroDWOSection()))
> > > parseMacroOrMacinfo(MacroDWO, OS, DumpOpts, /*IsLittleEndian=*/true,
> > >                               dwarf::MacroDWO);
> > > ```
> > > And it's not needed atleast for this patch.
> > > Should I go ahead remove this case and just leave a FIXME?
> > > 
> > If the behaviour from prior to your patch is unchanged, I think that's fine to do from my point of view. I wouldn't want to see things regress though.
> I think the problem is that you've put the FIXME above the case statement, whereas it should be below it. If you move it to before the break, it'll look right, and it's probably the right place for it:
> 
> ```
>   case dwarf::MacroDWO:
>     // FIXME: Add support for debug_macro.dwo section.
>     break;
> ```
Sure Thanks!
I'll take care of that!


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

https://reviews.llvm.org/D73086