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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Full disclosure: I'm coming from a system where there is only one ar tool, and using others isn't really supported. As such, I'm not bothered by adding new features to the tools that don't (yet) exist in GNU tools. If we do that more widely, we'll be held back in improving functionality by whatever the GNU community decide they want to accept. If we're going to start rejecting new options in llvm-ar, because GNU doesn't have/want them, we should start considering whether we should not be adding new options to llvm-objcopy/llvm-objdump/llvm-readelf etc, all of which I believe have options already that aren't in their GNU equivalents. Indeed, llvm-ar does too. It seems relatively unlikely to me that people will switch their binutils piecewise (although I could easily be wrong), so being concerned with one tool and not others seems inconsistent at least.

That all being said, I do agree that we should be careful with option naming. If it's not going to be accepted in other tools, we should be careful what we call the switch. "--output" would be an obvious example of a name that could clash, so discussing with other communities like GNU was the right course of action. I guess if they'd not wanted it, that shouldn't have stopped the option being added (if there was a good use-case for it), but it would have meant perhaps a different switch name that is less likely to clash in the future.



================
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:
> 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.


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

https://reviews.llvm.org/D69418