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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/docs/CommandGuide/llvm-objdump.rst:173
+  When disassembling with the :option:`--source` option, prepend ``prefix`` to
+  GNU absolute paths.
+
----------------
I don't think you need to mention GNU here. In general the concept of "absolute paths" is a little under-defined, plus people will likely expect the behaviour to match GNU.


================
Comment at: llvm/docs/llvm-objdump.1:111
+.Ar PREFIX
+to GNU absolute paths.
 .It Fl -print-imm-hex
----------------
Ditto.


================
Comment at: llvm/test/tools/llvm-objdump/X86/source-interleave-absolute-paths-all.test:1
+;; Test --prefix option.
+
----------------
Maybe just drop "-all" from the test name. In fact, the test probably just wants to be named something like "prefix.test" or possibly "source-interleave-prefix.test" to show it tests the prefix option (in general, tests should be named after the option they are testing). This second point applies to all the new tests.


================
Comment at: llvm/test/tools/llvm-objdump/X86/source-interleave-absolute-paths-all.test:3-10
+; RUN: sed -e "s,SRC_COMPDIR,/Inputs,g" %p/Inputs/source-interleave.ll > %t.ll
+; RUN: llc -o %t.o -filetype=obj -mtriple=x86_64-pc-linux %t.ll
+
+; RUN: sed -e "s,SRC_COMPDIR,%/p/Inputs,g" %p/Inputs/source-interleave.ll > %t2.ll
+; RUN: llc -o %t2.o -filetype=obj -mtriple=x86_64-pc-linux %t2.ll
+
+; RUN: sed -e "s,SRC_COMPDIR,,g" %p/Inputs/source-interleave.ll | sed -e "s,filename: \"source-interleave-x86_64.c\",filename: \"\",g"> %t3.ll
----------------
I'd prefer it if you created these inputs immediately before their first use rather than early on. It would also be good if you named the output files according to the property they are trying to show.


================
Comment at: llvm/test/tools/llvm-objdump/X86/source-interleave-absolute-paths-non-windows.test:1
+;; Test --prefix option non Windows specific behavior.
+; UNSUPPORTED: system-windows
----------------
This comment is ambiguous - it could imply behaviour that is not specific to Windows, which would also apply to the generic tests in the -all test.


================
Comment at: llvm/test/tools/llvm-objdump/X86/source-interleave-absolute-paths-windows.test:1
+; Test --prefix options on Windows.
+
----------------
tinti wrote:
> 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.
> 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 strongly disagree with making a test more general so that it isn't dependent on Windows behaviour - if we did that, we'd have no testing for Windows-specific behaviour which isn't good. I personally don't usually test my patches on Linux, as I almost exclusively develop on Windows. Does that mean I shouldn't write tests that are Linux-specific?

There are pre-merge bots that report test failures. Developers can and should rely on these to catch places where they aren't able to properly test things themselves locally.

To me, this test looks good.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:357
+cl::opt<std::string>
+    objdump::Prefix("prefix", cl::desc("Add prefix to GNU absolute paths"),
+                    cl::cat(ObjdumpCat));
----------------
Same as for the documentation.


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

https://reviews.llvm.org/D85024