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

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexshap 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())
----------------
jhenderson wrote:
> 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).
right, for the second check that would work, the problematic one is the first check ("Positional.empty()").


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87212