[PATCH] D62255: [llvm-objdump][test] Improve testing of some switches #3

[James Henderson via Phabricator via llvm-commits]

jhenderson marked 11 inline comments as done.
jhenderson added a comment.

Thanks for the comments. I'll update the patch again shortly.



================
Comment at: test/tools/llvm-objdump/X86/disasm-specific-funcs-mangled-name.test:6
+# RUN: llvm-objdump -d --disassemble-functions=_Z3foov %t.o | FileCheck %s
+# RUN: llvm-objdump -d --disassemble-functions=foo() %t.o | FileCheck %s --check-prefix=NOFOO
+# RUN: llvm-objdump -d -C --disassemble-functions=foo() %t.o | FileCheck %s --check-prefix=NOFOO
----------------
MaskRay wrote:
> jhenderson wrote:
> > rupprecht wrote:
> > > `--disassemble-functions=foo()` should be single quoted to handle (), e.g.
> > > 
> > > ```
> > > $ echo foo()
> > > bash: syntax error near unexpected token `('
> > > ```
> > I'll check this before committing. I had this vague impression that lit quotes the arguments, but I might be completely wrong.
> Even if lit supports it, quoting makes it easier to copy-n-paste the command.
> 
> In zsh it starts a function definition:
> 
> ```
> % echo foo()  
> function>
> ```
Single and double quotes both work (no quotes don't), so I'll update it.


================
Comment at: test/tools/llvm-objdump/X86/disasm-specific-funcs-mangled-name.test:13
+# CHECK-NEXT:  0000000000000000 _Z3foov:
+# CHECK-NEXT:        0: 90                            nop
+
----------------
grimar wrote:
> I would suggest to reduce amount of spaces here and below.
> Looks a bit better when compact:
> `0: 90 nop`
> 
> Up to you though.
How about I get rid of the 90 and nop completely? I don't think they're needed for the test.


================
Comment at: test/tools/llvm-objdump/X86/disasm-specific-funcs.test:15
+# FOO-NEXT:           c: c3                            retq
+# FOO-NEXT:           d: 0f 1f 00                      nopl    (%rax)
+
----------------
grimar wrote:
> Remove excessive spaces between `FOO-NEXT:` and addresses?
I also removed the actual disassembly from the test check, since I don't see a need for it.


================
Comment at: test/tools/llvm-objdump/X86/disassemble-long-instructions.test:1
+## This test shows that llvm-objdump can print the disassembly a long instruction.
+# RUN: yaml2obj %s -o %t.o
----------------
MaskRay wrote:
> > print the disassembly a long instruction.
> 
> Should this be reworded a bit?
> 
> 
> So we have both `disasm-*` and `disassemble-*` tests... You may rename the existing ones and rename this to `disasm-*` if you want to keep consistency.
I'll do the renaming of files that don't start with "elf-" in a follow-up patch, as we have quite a mixture already.


================
Comment at: test/tools/llvm-objdump/X86/elf-disassembly-no-symtab.test:1
+## Show that llvm-objdump can handle a missing symbol table when printing
+## references and labels.
----------------
MaskRay wrote:
> This patch adds
> 
> `elf-disassembly-no-symtab.test`
> `elf-disassemble-relocs.test`
> `elf-disasm-dynamic-symbols.test`
> 
> I hope the inconsistency is not intentional..
Believe it or not, I think there was some intention behind it but I cannot remember what! I think it depended on which feature or switch I was testing in particular. But I agree that inconsistency is unhelpful and will rename them.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62255