[PATCH] D68730: [llvm-objdump] Adjust spacing and field width for --section-headers

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/test/tools/llvm-objdump/section-headers-address-width.test:10
+# 32: {{^}}Idx Name          Size     VMA      LMA      Type{{$}}
+# 32: {{^}}  1 .foo          00000000 00000400 00000400 TEXT{{$}}
+
----------------
I wonder, should we be able to shrink it too?

Something like:
Len = max length among all sections.

Instead of a current (if I understand it right)
Len = max(max length ..., 13)


================
Comment at: llvm/test/tools/llvm-objdump/section-headers-address-width.test:37
+    Flags:        [SHF_ALLOC, SHF_EXECINSTR]
+    Address:      0x400
+...
----------------
nit: We are often reducing the identation to a single space after the longest tag. Just like you have for `FileHeader`
(here and everywhere below)


================
Comment at: llvm/test/tools/llvm-objdump/section-headers-address-width.test:38
+    Address:      0x400
+...
+
----------------
nit: you can remove '...'
(here and everywhere below)


================
Comment at: llvm/test/tools/llvm-objdump/section-headers-name-width.test:2
+## The name field automatically expands past the default 13 columns when a
+## section name is longer than that.
+# RUN: yaml2obj %s --docnum=1 -o %t-name13chars.o
----------------
nit: missing empty line after comment.


================
Comment at: llvm/test/tools/llvm-objdump/section-headers-name-width.test:4
+# RUN: yaml2obj %s --docnum=1 -o %t-name13chars.o
+# RUN: llvm-objdump -h --show-lma %t-name13chars.o \
+# RUN:   | FileCheck %s --check-prefix=NAME-13 --strict-whitespace
----------------
You are not using `--show-lma` it seems? I.e. I am not sure why do you need this test.


================
Comment at: llvm/test/tools/llvm-objdump/section-headers-name-width.test:16
+
+# RUN: yaml2obj %s --docnum=2 -o %t-name14chars.o
+# RUN: llvm-objdump -h --show-lma %t-name14chars.o \
----------------
nit: We often add a short comment for each test in addition to a file comment.
I.e. you could say something like "here we test that name with the length 14 expands ..."


================
Comment at: llvm/test/tools/llvm-objdump/section-headers-spacing.test:1
+## Check leading and trailing whitespace for full lines.
+# RUN: yaml2obj %s -o %t-whitespace.o
----------------
What do you think about combining these tests you have here into one that
could use `yaml2obj --docnum=X` and check spacing, formatting etc in one place?
(I am not sure it if it is usefull to have 3 different test files?)


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1653
 
+static size_t maxNameWidth(const ObjectFile *Obj) {
+  // Default column width for names is 13 even if no names are that long.
----------------
`getMaxSectionNameWidth`?


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1694
+      SectionTypes.push_back("BSS");
+    std::string Type = llvm::join(SectionTypes, " ");
 
----------------
May be I'd try to avoid using an additional vector and algorithm here and just:

```
  std::string Type = Section.isText() ? "TEXT" : "";
  if (Section.isData())
    Type += Type.empty() ? "DATA" : " DATA";
  if (Section.isBSS())
    Type += Type.empty() ? "BSS" : " BSS";
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68730