[PATCH] D85024: [llvm-objdump] Implement --prefix option

[Vinicius Tinti via Phabricator via llvm-commits]

tinti added inline comments.


================
Comment at: llvm/test/tools/llvm-objdump/X86/source-interleave-absolute-paths-windows.test:1
+; Test --prefix options on Windows.
+
----------------
MaskRay wrote:
> Tests which only run on Windows should be added very carefully. Many developers don't have Windows testing method. When they do refactoring, they can easily neglect Windows-only tests. `/\` has some pain but you can get round with `{{[/\\]}}`
I just found the error because the **buildbot** pointed it.

I don't have too a Windows testing method. I am thinking on removing this test and add only a comment depending on the behavior of **GNU's objdump** on Windows.


================
Comment at: llvm/test/tools/llvm-objdump/X86/source-interleave-absolute-paths.test:7
+; RUN: llc -o %t.o -filetype=obj -mtriple=x86_64-pc-linux %t.ll
+; RUN: llvm-objdump --source %t.o 2>&1 | FileCheck %s
+
----------------
jhenderson wrote:
> In both tests, it probably makes sense to only have a `--prefix` case - there is existing testing for `--source` so we don't need to show that behaviour works. I also think you need a case where you have `--prefix` and it allows you to find the path. Perhaps you could use just `/Inputs` in the sed line, and then use `--prefix` to add `%/p` to the path.
Agreed. I will try.


================
Comment at: llvm/test/tools/llvm-objdump/X86/source-interleave-absolute-paths.test:9
+
+; CHECK: 0000000000000000 <foo>:
+; CHECK-NEXT: ; int foo() {
----------------
jhenderson wrote:
> When using a mixture of CHECK and CHECK-NEXT lines, it's a little more readable to me if you indent the CHECK lines so that their value start lines up with those for CHECK-NEXT:
> ```
> ; CHECK:      0000000000000000 <foo>:
> ; CHECK-NEXT: ; int foo() {
> ```
Agreed.


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

https://reviews.llvm.org/D85024