[PATCH] D137096: [llvm-readobj] Provide Hash Histogram for all ELFDumper implementations

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/hash-histogram.test:16-17
+## Check that LLVM output has the expected format.
+# RUN: llvm-readobj --histogram %t1-32.o | FileCheck %s --check-prefix=LLVM-HIST
+# RUN: llvm-readobj --histogram %t1-64.o | FileCheck %s --check-prefix=LLVM-HIST
+# RUN: llvm-readobj --elf-hash-histogram %t1-32.o | FileCheck %s --check-prefix=LLVM-HIST
----------------
There's no need for these two extra cases: they are there for the earlier llvm-readelf check to ensure the --histogram and -I aliases are equivalent to --elf-hash-histogram.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:7222
+    ArrayRef<size_t> Count) const {
+  DictScope Outer(W, "HashHistogram");
+  W.printNumber("TotalBuckets", NBucket);
----------------
Looking at this again, this and the GNU hash histogram version immediately below are practically identical. Can you try sharing the code rather than duplicating it, please?

Whilst you're at it, you might want to do the same for the GNU output format versions of these two functions.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:7155
+  size_t CumulativeNonZero = 0;
+  for (size_t I = 0; I < MaxChain; I++) {
+    CumulativeNonZero += Count[I] * I;
----------------
paulkirth wrote:
> jhenderson wrote:
> > https://llvm.org/docs/CodingStandards.html#prefer-preincrement
> This loop is taken directly from the `GNUELFDumper`, should I update this in the existing implementations too? 
I wouldn't change code that isn't already changing/moving, but if it is, you can make the tidy-up at the same time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137096