[PATCH] D46407: [tools] Introduce llvm-strip

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: tools/llvm-objcopy/CMakeLists.txt:10
 tablegen(LLVM Opts.inc -gen-opt-parser-defs)
-add_public_tablegen_target(ObjcopyTableGen)
+add_public_tablegen_target(OptsTableGen)
+
----------------
I'm not really familiar with cmake, but does this name have to be unique? If so, we might want to go back to the old name.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:487-490
+  if (InputArgs.size() == 0 || InputArgs.hasArg(STRIP_help)) {
+    T.PrintHelp(outs(), "llvm-strip <input> [ <output> ]", "strip tool");
+    exit(0);
+  }
----------------
I think the norm is to exit(1) if the input count is 0, unless help has been specified.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:498-499
+
+  if (Positional.empty())
+    error("No input file specified");
+
----------------
Test case?


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:519
+  CopyConfig Config;
+  if (sys::path::stem(ToolName).endswith_lower("strip"))
+    Config = ParseStripOptions(makeArrayRef(argv + 1, argc));
----------------
I'm not a massive fan of using the tool's filename to determine the expected behaviour (example reason: if I change the executable from llvm-strip to llvm-strip-backup, when testing two versions of the tool side-by-side, I'd switch to objcopy style behaviour unexpectedly).

How feasible would it be to build two different executables, using the same source code, but a command-line specified define to control this?


Repository:
  rL LLVM

https://reviews.llvm.org/D46407