[PATCH] D102665: [llvm-objcopy] Add support for '--' for delimiting options from input/output files

[James Henderson via Phabricator via llvm-commits]

jhenderson requested changes to this revision.
jhenderson added a comment.
This revision now requires changes to proceed.

I'm not against the functional change, but why have so many tests changed? You should have a single dedicated test that covers this, rather than arbitrarily modifying existing tests to cover the new behaviour. As this is not file format specific, it can even be put in the top-level llvm-objcopy directory, although you may want a comment saying it is testing generic behaviour, since it'll presumably need to use e.g. an ELF object to avoid the "no input file" error.

Is there a particular reason you've singled out llvm-objcopy, and not llvm-strip?



================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:478
+
+  auto DashDash = std::find_if(RawArgsArr.begin(), RawArgsArr.end(),
+                               [](StringRef Str) { return Str == "--"; });
----------------
Nit: fix this clang-tidy issue.


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:486
 
-  if (InputArgs.size() == 0) {
+  if (InputArgs.size() == 0 && DashDash == RawArgsArr.end()) {
     printHelp(T, errs(), ToolType::Objcopy);
----------------
What about the case where a user does `llvm-objcopy --` with no arguments or input files? What does GNU objcopy do in this case?


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

https://reviews.llvm.org/D102665