[PATCH] D51009: [opt] Change the parameter of OptTable::PrintHelp from Name to Usage and don't append "[options] <inputs>"

Fāng-ruì Sòng via llvm-commits llvm-commits at lists.llvm.org
Tue Oct 9 17:58:44 PDT 2018


On 2018-10-10, Jake Ehrlich via Phabricator wrote:
>jakehehrlich added a comment.
>
>In https://reviews.llvm.org/D51009#1259745, @MaskRay wrote:
>
>> .
>
>
>Please don't remove subscribers like this. People setup Herald rules to make sure they're notified of any changes. Also, are you sure you've added all the needed reviewers for this? This is touching a lot of tools and this is not really the diverse group of reviewers I'd expect to give the ok on this sort of stuff.

Sorry if I have removed subscribers. It was probably because I misused
or forgot to use `arc amend` to pull reviewers/subscribers lines from
Phabricator to my local git commit.

I only know the following two and probably misused them.

* arc amend
* arc diff --verbatim

>The intent of this change seems non-offensive to me and I'm generically fine with it.
>
>Also "<inputs>" seems better than "files..." to me.

GNU binutils use <param> but I don't think we need to be consistent with
them here. As what I have observed:

FreeBSD nm: Usage: nm [options] file ...
FreeBSD strip: Usage: strip [options] file...

GNU strip: Usage: strip <option(s)> in-file(s)
GNU objdump: Usage: objdump <option(s)> <file(s)>
GNU cat: Usage: cat [OPTION]... [FILE]...
GNU dd: Usage: dd [OPERAND]...
GNU rev: Usage: rev [options] [file ...]

The <param> form seems a minority. < > can be confused with file
descriptor redirection. While [ ] can also be misguiding, but I think
people have accepted that [ ] are used to represent optional arguments.

>
>
>
>================
>Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:900
>   if (InputArgs.size() == 0) {
>-    T.PrintHelp(errs(), "llvm-objcopy <input> [ <output> ]", "objcopy tool");
>+    T.PrintHelp(errs(), "llvm-objcopy input [output]", "objcopy tool");
>     exit(1);
>----------------
>rupprecht wrote:
>> extract this to a static const char[] like for strip below
>Maybe we should add "[options]" before "input"

Reasonable. I'll do that in another revision.

>
>Also, I think I'd prefer "<input>" to "input"
>
>
>================
>Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:1042
>
>+  static const char Usage[] = "llvm-strip [options] file...";
>   if (InputArgs.size() == 0) {
>----------------
>I'd prefer "<inputs>" to "file..."
>

-- 
宋方睿


More information about the llvm-commits mailing list