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

[Igor Kudrin via Phabricator via llvm-commits]

ikudrin added inline comments.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:24
+               getVersion(), getFlags());
+  if (getFlags() == MACRO_DEBUG_LINE_OFFSET)
+    OS << format(", debug_line_offset = 0x%04" PRIx64 "\n",
----------------
Please, use a canonical way to check flags.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:155
+  // FIXME: Add support for DWARF64
+  if (Flagdata == MACRO_OFFSET_SIZE) {
+    Error Err =
----------------
SouraVX wrote:
> probinson wrote:
> > You want to check only the single bit here, for example:
> > `if (Flagdata & MACRO_OFFSET_SIZE == 1)`
> > 
> Thanks for this! I'll incorporate this in next revision alongside concerns of other reviewers if remaining.
> `if (Flagdata & MACRO_OFFSET_SIZE == 1)`

Probably, without `== 1`, because this is an explicit constant which should be avoided: `if (Flagdata & MACRO_OFFSET_SIZE)`.

Or, at least, `if (Flagdata & MACRO_OFFSET_SIZE == MACRO_OFFSET_SIZE)`, but personally I'd prefer the compact variant.

These two are canonical ways to check flags.




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

https://reviews.llvm.org/D73086