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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

I agree that inventing an option that could clash with future GNU addition should be avoided. We can ask GNU people whether they'd like to add the same functionality. Their rejection should not block our innovation, because we may still get a commitment from them that they will not intentionally use the same option for an incompatible feature. It seems the GNU ar commit as submitted by Nick Clifton only works for `relative/dir` not `/absolute/dir` or `..` (my original one works for these case), I'll work with him to fix the problem, but in any case we can proceed with the llvm-ar change once the review concerns are all addressed.



================
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
----------------
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`?




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

https://reviews.llvm.org/D69418