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

[David Blaikie via Phabricator via llvm-commits]

dblaikie 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);
----------------
SouraVX wrote:
> 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!
> I've tied that part also, that result's in some warning of uninitialized variable warning @ikudrin point that out.

I'm not sure how that could be a problem there, but not here & I couldn't find any mention of that uninitialized variable warning discussion in this code review thread - could you point me to that discussion so I can better understand the issues and if/how they apply there but not here?

> I thinks this explicit approach(passing section name) rather than implicit implementation is clear/straight-forward, specially in distinguishing part of macro/macinfo section.

A few issues - mostly that passing explicitly written strings on one side and comparing them to explicitly written strings on the other side is error-prone (easy to typo & have no compiler error/warning to catch it) & inefficient (doing entire string comparisons to test a two-state situation). And the inconsistency between this code that does test the version from the header and the code 10 lines away that does string comparison.



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

https://reviews.llvm.org/D73086