[PATCH] D62973: [llvm-objcopy] Changed command line parsing errors

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:666-667
 
+  if (Config.OutputFilename == "-" || Config.InputFilename == "-")
+    if (Config.PreserveDates)
+      return createStringError(errc::invalid_argument,
----------------
These can be combined, i.e. `if (Config.PreserveDates && (Config.OutputFilename == "-" || Config.InputFilename == "-"))`


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:806-807
 
+  if (Config.OutputFilename == "-" || Config.InputFilename == "-")
+    if (Config.PreserveDates)
+      return createStringError(errc::invalid_argument,
----------------
strip is a little more exotic in the way it accepts args; objcopy only supports:
```
$ llvm-objcopy foo  # "copies" in place
$ llvm-objcopy foo bar
$ llvm-objcopy foo bar baz # not valid
```

Whereas strip supports a couple variants, including N args:
```
$ llvm-strip foo  # strips in place
$ llvm-strip foo bar baz ... # also strips N objects in place
$ llvm-strip foo -o bar # strips foo, saving it as bar
```

and the way it's handled above (with `DriverConfig`) is that we create one `CopyConfig` with all the non-filename args, and then modify it in-place when we copy it into `DriverConfig`. So, `Config.OutputFilename` here is really just the *last* filename. (e.g. try this: `llvm-strip -p - foo`).

An alternative for strip (and maybe we could use the same thing for objcopy, I don't have a preference) would be to just check directly against the positional args:
```
if (Config.PreserveDates && (llvm::is_contained(Positional, "-") || InputArgs.getLastArgValue(STRIP_output) == "-"))
```

(see `include/llvm/ADT/STLExtras.h`)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62973