[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:
> > 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()").
As it's only an interim state, I'd either delete the first check or replace it with an assert (on the basis that it's currently dead). I don't think it really matters either way, but I'd prefer making it something simple, in case e.g. there's a bug in the `createStringError` usage, which will need fixing up anyway once we have a way to exercise it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87212