[PATCH] D87212: [llvm-objcopy][MachO] Add llvm-bitcode-strip driver

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

It probably makes sense to include a link to the RFC for this in the patch description/commit message. It might also be a good idea to start on an .rst doc for this too, to avoid it getting forgotten about, though maybe it's worth waiting for the first "useful" option to be implemented.



================
Comment at: llvm/test/tools/llvm-objcopy/tool-help-message.test:28-38
 # OBJCOPY-USAGE:  USAGE: llvm-objcopy [options] input [output]
 # OBJCOPY-USAGE:  Pass @FILE as argument to read options from FILE.
 
 # STRIP-USAGE:    USAGE: llvm-strip [options] inputs...
 # STRIP-USAGE:    Pass @FILE as argument to read options from FILE.
 
 # INSTALL-NAME-TOOL-USAGE: USAGE: llvm-install-name-tool [options] input
----------------
Optional, but it might be worth refactoring this test to avoid the duplicate check patterns for the response file bit (i.e. something like `--check-prefix=OBJCOPY-USAGE,RSP-FILE`).


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:973-975
+// ParseBitcodeStripOptions returns the config and sets the input arguments.
+// If a help flag is set then ParseBitcodeStripOptions will print the help
+// messege and exit.
----------------
I know this is done elsewhere, but maybe we don't need a comment in both the header and source file, especially as the behaviour isn't exactly unexpected.


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:985-990
+  if (MissingArgumentCount)
+    return createStringError(
+        errc::invalid_argument,
+        "missing argument to " +
+            StringRef(InputArgs.getArgString(MissingArgumentIndex)) +
+            " option");
----------------
I think we should avoid adding code like this that can't currently be exercised, until we add an option that requires it. That way, we can ensure there's adequate testing for the code path at the time functionality is added.


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:1009
+
+  for (auto Arg : InputArgs.filtered(BITCODE_STRIP_UNKNOWN))
+    return createStringError(errc::invalid_argument, "unknown argument '%s'",
----------------
Probably worth either removing the `auto` here, or at least doing what clang-tidy wants. Same below.


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:1013-1025
+  SmallVector<StringRef, 2> Positional;
+  for (auto Arg : InputArgs.filtered(BITCODE_STRIP_INPUT))
+    Positional.push_back(Arg->getValue());
+  // TODO: Add tests for these errors once we implement support for the
+  // tool-specific command line options. At the moment we do not reach these
+  // cases because of early returns.
+  if (Positional.empty())
----------------
Same comment as above. I'd avoid adding code that can't currently be hit, and wait to add it until the functionality is required.


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.h:273
+// messege and exit.
+Expected<DriverConfig>
+parseBitcodeStripOptions(ArrayRef<const char *> ArgsArr);
----------------
clang-format nit. Also in CopyConfig.cpp.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87212