[PATCH] D81907: [llvm-objcopy] Fix help text

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

LGTM, with one nit.



================
Comment at: llvm/test/tools/llvm-objcopy/ELF/help-message.test:15
 
 
+# OBJCOPY-USAGE:  USAGE: llvm-objcopy [options] input [output]
----------------
Nit: whilst you're here, please delete one of these two blank lines.


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/install-name-tool-help-message.test:1
-# RUN: llvm-install-name-tool -h | FileCheck --check-prefix=INSTALL-NAME-TOOL-USAGE %s
-# RUN: llvm-install-name-tool --help | FileCheck --check-prefix=INSTALL-NAME-TOOL-USAGE %s
-# RUN: not llvm-install-name-tool 2>&1 | FileCheck --check-prefix=INSTALL-NAME-TOOL-USAGE %s
+# RUN: llvm-install-name-tool -h | FileCheck --check-prefix=INSTALL-NAME-TOOL-USAGE %s --match-full-lines
+# RUN: llvm-install-name-tool --help | FileCheck --check-prefix=INSTALL-NAME-TOOL-USAGE %s --match-full-lines
----------------
So I just noticed that neither this nor the llvm-objcopy/llvm-strip test belong in their current directories - they should be one level up, since they are not platform specific, and probably then merged into a single test in my opinion. No need to change it in this patch, but if you can afford the hopefully small amount of time to do it, I think merging the two tests and putting them in `llvm/test/tools/llvm-objcopy/` would be a nice thing to do.

Note that `tool-name.test` already tests all three tools.


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:398
                       StringRef ToolName) {
-  OptTable.PrintHelp(OS, (ToolName + " input [output]").str().c_str(),
+  StringRef Output = ToolName == "llvm-install-name-tool" ? "" : " [output]";
+  OptTable.PrintHelp(OS, (ToolName + " input" + Output).str().c_str(),
----------------
sameerarora101 wrote:
> jhenderson wrote:
> > Rather than relying on a string for the name, I wonder if this code would be more robust with an enum that represents the three different tools, given that there's always potential for more front ends to be added. You would probably still want to pass in the tool name, but that could just be whatever the executable is called, possibly. For example, if the tool was llvm_install_name_tool, it would print that as the name in the help text message, rather than the current llvm-install-name-tool.
> So I made a small enum and passed the appropriate enum types at the various function calls to `printHelp`. Does this work?
Yup, thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81907