Tidied up errors during command line parsing to be more consistent with the rest of llvm-objcopy errors.
|1 ↗||(On Diff #203422)|
This test is redundant w/ strip-preserve-atime.test and strip-preserve-mtime.test, so you can remove it.
Flag validation errors should go in CopyConfig.cpp whenever possible so we can do simple validation as early as possible.
These can be combined, i.e. if (Config.PreserveDates && (Config.OutputFilename == "-" || Config.InputFilename == "-"))
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) == "-"))
I'm good with this after these two comments:
|2 ↗||(On Diff #203458)|
This actually isn't being run because there is no FileCheck command above. The commands should be like:
# RUN: not llvm-objcopy --preserve-dates - %t < %p/Inputs/alloc-symtab.o 2>&1 | FileCheck %s
|4 ↗||(On Diff #203458)|
Can you add some cases for strip?
# Testing N args not llvm-strip --preserve-dates - not llvm-strip --preserve-dates %p/Inputs/alloc-symtab.o - not llvm-strip --preserve-dates - %p/Inputs/alloc-symtab.o not llvm-strip --preserve-dates %p/Inputs/alloc-symtab.o - %p/Inputs/alloc-symtab.o # Testing -o not llvm-strip --preserve-dates - -o %p/Inputs/alloc-symtab.o not llvm-strip --preserve-dates %p/Inputs/alloc-symtab.o -o -
(and others if you think of any)
Tidied up errors during command line parsing to be more consistent with the rest of llvm-objcopy errors. Also, ensure --preserve-dates is only ever used when both input and output files are regular files.
I haven't looked in depth, and I expect there's nothing wrong with these changes, but really they should be two separate commits. One for "tidying up error messages" and the other for the --preserve-dates check. Could you split them up before committing, please?
Also a quick tip for when writing tests for bug fixes: make sure the test fails prior to your change in the way you'd expect it to. This would have picked up the missing FileCheck, above I believe.