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

Fri Oct 11 10:05:42 PDT 2019

rupprecht 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
----------------
grimar wrote:
> 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).
I don't have that strong of preference either, so merged. My only concern is now it's on the larger end of test sizes (7th of all objdump tests per `find llvm/test/tools/llvm-objdump/ -name '*.test' | xargs wc -l | sort -nr | tail +2 | head -10`), but it's still not excessively long.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68730