[PATCH] D69418: [llvm-ar] Add output option for extract operation

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-ar/extract.test:28
+RUN: diff %t/a.txt %t/extracted/a.txt
+RUN: diff %t/b.txt %t/extracted/b.txt
----------------
MaskRay wrote:
> jhenderson wrote:
> > MaskRay wrote:
> > > kongyi wrote:
> > > > MaskRay wrote:
> > > > > We probably also need a test that `--output` is after the extracted member. This also works. (e.g. `ar x a.a a.txt --output=dir`)
> > > > The help message specifies that options has to be placed before the operation flags. Although we accept this, I don't think a test is necessary to assert the undefined behaviour.
> > > I don't have more comments. Probably worth waiting a bit for another opinion.
> > > 
> > > For shell scripts that do `llvm-ar x a.a $x`, they probably should switch to `llvm-ar x -- a.a "$x"`, which is safer if `$x` can be weird things like `--output /home/user`.
> > I'd be wary about calling it undefined behaviour. It's common for usage to list options then inputs, but that doesn't mean specifying them the other way around is undefined in other tools. Indeed, it's not uncommon for people to do something like "llvm-readelf input.o --sections" or similar. If we want to require "--output" (and I'd add the other similar arguments for consistency too) be before the command, it should be more explicitly stated in both the help text and command guide. Speaking of which, please add the option to the llvm-ar CommandGuide documentation.
> > 
> > I'm relatively ambivalent as to whether this and other long options should be allowed afterwards. I'd lean towards not for llvm-ar because of its weird command-line rules. I also feel like this should be enforced and tested, to stop people relying on such behaviour. In other words, testing that --output after the archive name would attempt to add/extract etc a file called "--output". When we can easily avoid exposing undefined behaviour, I think we should.
> > I'm relatively ambivalent as to whether this and other long options should be allowed afterwards.
> 
> To clarify, do we want to support the following forms
> 
> ```
> llvm-ar x --output=dir a.a  # An existing test archive-symtab.test does `llvm-ar rcs --format=bsd %t.a`
> llvm-ar --output=dir x a.a
> ```
> 
> but not `llvm-ar x a.a --output=dir`?
> 
> 
I don't have strong feelings on it, but if I were to cast a vote, I'd go go with that suggestion (not allow `--output=dir` afterwards). My reasoning is that not allowing `llvm-ar x a.a --output=dir` may in fact be easier in some ways: how else do you pass in filenames starting with `-` after all? The example you gave above of the need to switch to adding `--` is an example of why it may be better to not to allow it.

Like I said though, no strong feelings, so if others prefer a different approach, that's cool with me.


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

https://reviews.llvm.org/D69418