[PATCH] D61755: llvm-dwarfdump: Add dwo parsing to --statistics.

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

Looks good - thanks!



================
Comment at: include/llvm/DebugInfo/DWARF/DWARFUnit.h:390-394
+    if (DWO) {
+      return DWO->getUnitDIE(ExtractUnitDIEOnly);
+    } else {
+      return getUnitDIE(ExtractUnitDIEOnly);
+    }
----------------
LLVM style tends to omit {} on single line scopes, and to avoid using 'else' after a return or similar ( https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return ).

So this could be modified to:

  if (DWO)
    return DWO->getUnitDIE
  return getUnitDIE...

Alternatively, could use the conditional operator, ala:

  return (DWO ? *DWO : *this).getUnitDIE(...);

(similar to the other use of parseDWO/pick-the-right-unit in DWARFUnit::getInlinedChainForAddress)


================
Comment at: test/tools/llvm-dwarfdump/X86/statistics-dwo.test:11
+
+# Source program - A version of Fibonacci
+# Compilation options:  -gsplit-dwarf -O3 -c -S
----------------
(I'd generally suggest smaller/more targeted tests - rather than "here's a bunch of code and the statistics we expect for it" (a test that's small enough that most of the stats (the ones that aren't related to instruction counts) could be eyeballed/manually computed/verified easily enough - for instance the first test for statistics (test/tools/llvm-dwarfdump/X86/statistics.ll) explains the values of the various variable statistics)

Having a test with features designed for particular functionality makes test intentions easier to follow (for me, at least) - if bugs are found and numbers change or new statistics are added, it'll be easier to tell which parts of the behavior are intentional and which parts might've been accidentally over-fitted etc.)

But no worries in this instance - thoughts for future work.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61755