[PATCH] D66134: [llvm-size][test] Improve llvm-size testing

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: test/tools/llvm-size/archive.test:37
+# BERKELEY-3-NEXT:   1       2       4       7       7 [[FILE1]]
+# BERKELEY-3-NEXT:   1       2       4       7       7 archive.test.tmp1 (ex [[ARCHIVE2]])
+# BERKELEY-3-NEXT:   8      16      32      56      38 [[FILE2]]
----------------
`archive.test.tmp1`? Should it be `[[FILE1]]`?


================
Comment at: test/tools/llvm-size/common.test:8
+# RUN: llvm-size -A %t.o | FileCheck --check-prefix=SYSVNOCOMM %s
+# RUN: llvm-size -B %t.o | FileCheck --check-prefix=BSDNOCOMM %s
 
----------------
nit: since `--common` is not the default, perhaps it should be `SYSV` + `SYSVCOMMON`?
Up to you.


================
Comment at: test/tools/llvm-size/common.test:48
+    Size:  4
+    Index: SHN_COMMON
----------------
I'd use just 2 symbols of a different sizes probably.


================
Comment at: test/tools/llvm-size/elf-m.test:34
+    Size:    4
+    Address: 1
----------------
I am not sure you need any sections to show that the output format changed.
I.e. I guess that having an ELF yaml header should be enough here.


================
Comment at: test/tools/llvm-size/elf-sysv.test:108
+    Address: 0x40
+    Info:    a_symbol
+    Members:
----------------
You can probably just do: `Info: 0`?


================
Comment at: test/tools/llvm-size/invalid-input.test:9
+# RUN: not llvm-size %s 2>&1 | FileCheck %s -DFILE=%s --check-prefix=UNRECOGNIZED
+# UNRECOGNIZED: {{.*}}llvm-size{{(.*)}}: [[FILE]] The file was not recognized as a valid object file
+
----------------
It doesn't print the "error: " prefix, right? This is a bit strange behavior.


================
Comment at: test/tools/llvm-size/long-format.test:4
+
+# RUN: llvm-size %p/Inputs/darwin-m.o -m -l | FileCheck %s
+
----------------
How much is hard/reasonable to switch to using a YAML instead of the precompeiled `darwin-m.o`?


================
Comment at: test/tools/llvm-size/macho-sysv.test:10
+# CHECK-NEXT: __data         4      4
+# CHECK-NEXT: Total          8
----------------
`# CHECK-NOT:{{.}}` perhaps?


================
Comment at: test/tools/llvm-size/no-input.test:4
+# RUN: not llvm-size %t.blah 2>&1 | FileCheck %s -DFILE=%t.blah --check-prefix=ENOENT
+# ENOENT: {{.*}}llvm-size{{(\.EXE|\.exe)?}}: [[FILE]] {{[Nn]}}o such file or directory
+
----------------
I wonder if it is really important to check the tool executable name?
For other tools we often just check the "error: filename: text", I am not sure
that testing the `{{.*}}llvm-size{{(\.EXE|\.exe)?}}` makes a lot of sense? (looks a bit complicated and excessive)


================
Comment at: test/tools/llvm-size/radix.test:108
+## Case 9: Bad values.
+# RUN: not llvm-size %t1.o %t2.o --radix=0 2>&1 \
+# RUN:   | FileCheck %s --check-prefix=BAD-VAL -DNUM=0
----------------
You can have a single input file for these tests here probably.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66134