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

[Sourabh Singh Tomar via Phabricator via llvm-commits]

SouraVX marked an inline comment as done.
SouraVX added inline comments.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:47
+      // Based on which version we are handling choose appropriate macro forms.
+      if (Macros.Header.getVersion() >= 5)
+        WithColor(OS, HighlightColor::Macro).get() << MacroString(E.Type);
----------------
dblaikie wrote:
> If we can test the version here - please use the same version test above (for choosing whether to dumpMacroHeader) rather than passing in a string.
> 
> (is the version correctly initialized (at least to zero or /something/ not uninitialized) when parsing debug_macinfo/pre-v5 format that doesn't have a header - but that would be necessary for this code already, as well as for removing the use fo the SectionName parameter)
I've tied that part also, that result's in some warning of uninitialized variable warning @ikudrin point that out. I thinks this explicit approach(passing section name) rather than implicit implementation is clear/straight-forward, specially in distinguishing part of macro/macinfo section. 
While parsing we just have(in this case) `Context` and `Extractor` none of which can render the information the section macro/macinfo section we're parsing and as mentioned above `getMaxVersion` won't be much useful here since it will parse/traverse all info units for Maximum version present. Therefore, I prefer passing the `SectionName` it self.
Passing `Section Name` or `Version`(4 or 5 as you mentioned) is still debatable? For readability and all.  (personal choice) will be to pass `SectionName` -- easy for user to correlate which section has header instead of numbers? 
Please share your opinion on this!


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

https://reviews.llvm.org/D73086