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

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht marked 9 inline comments as done.
rupprecht 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{{$}}
+
----------------
grimar wrote:
> 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)
13 is chosen to make the output match GNU objdump. Having a cutoff (not necessarily 13, but any number around there) also makes the formatting stable for objects with regular-length section names.


================
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
----------------
grimar wrote:
> You are not using `--show-lma` it seems? I.e. I am not sure why do you need this test.
Added a comment -- the reason is just that it goes down a different code path, e.g. would catch a bad implementation like:
```
if (ShowLMA)
  outs() << "Name    " << ... // Oops, didn't use left_justify
else
  outs() << left_justify("Name", NameWidth) << ...
```


================
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
----------------
grimar wrote:
> 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?)
I started out with one test file, but found it to be a collection of somewhat unrelated things -- e.g. name column width and 32 vs 64 bit column widths are different features. So I think it's better to have more focused test files. It's a slightly personal preference though.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1694
+      SectionTypes.push_back("BSS");
+    std::string Type = llvm::join(SectionTypes, " ");
 
----------------
grimar wrote:
> 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";
> ```
I think the lack of using an algorithm is why there was odd trailing whitespace before, although I agree it would be great if this could be more succinct...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68730