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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
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())
----------------
alexshap wrote:
> jhenderson wrote:
> > 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.
> another option is to call "exit(0)" - without these checks InputfFilename/OutputFilename can't be safely set, and without them one can not construct a valid instance of Config.  Not sure if exit(0) inside the function which parses command-line options is better, but it doesn't matter much anyway. What's your preference ?
Actually, can't this code be hit with something like `llvm-bitcode-strip input.o output.o` (i.e. no options)?

In that case, you could test using that. The inputs would just have to be enough to get us through this code/exercise the errors safely (so might need to be arbitrary ELF objects, or possible even just empty files for now).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87212