[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-spacing.test:1
+## Check leading and trailing whitespace for full lines.
+# RUN: yaml2obj %s -o %t-whitespace.o
----------------
rupprecht wrote:
> 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.
We often combine tests by a feature. I.e. test that checks the "-h" output might contain everything related to "-h" at once. Sometimes we do a split to make a test that contain only a error/warnings checks (if there are too many of them) or a particular set of tests that are very different.

It is not a huge problem, but might be interesting what others think about this too though.
(to summarize my position: I'd prefer to combine them to reduce the number of tests, but it is not critical and is OK as is probably).


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1694
+      SectionTypes.push_back("BSS");
+    std::string Type = llvm::join(SectionTypes, " ");
 
----------------
rupprecht wrote:
> 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...
But does my version has a trailing whitespace issue? I think no.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68730