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

[MaskRay]

Summary

Before, "[options] <inputs>" is unconditionally appended to the Name parameter. It is more flexible to change its semantic to Usage and let user customize the usage line.

% llvm-objcopy
...
USAGE: llvm-objcopy <input> [ <output> ] [options] <inputs>

With this patch:

% llvm-objcopy
...
USAGE: llvm-objcopy input [output]

[MaskRay]

Newline

[rupprecht]

Some uses of this already use a space and probably don't want this behavior -- e.g. tools/clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp invokes this w/ "clang -cc1". (There are a few others).

It seems like just checking for a space isn't a perfect signal for if this is a tool name or a full usage. Maybe there's another way, such as configuring (via a method or constructor arg in OptTable) whether the tool supports multiple inputs or multiple outputs?

Whatever you come up with, can you add a test case to unittests/Option/OptionParsingTest.cpp?

[MaskRay]

Add test to unittests/Option/OptionParsingTest.cpp

[MaskRay]

tools/clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp invokes this w/ "clang -cc1".

Thanks for pointing this out. The other one is clang -cc1as. I think the best way is to make the two instances explicit https://reviews.llvm.org/D51109
I have checked other references and think they are fine with the OptTable::PrintHelp change.

[MaskRay]

Will it be good to update all the call sites of OptTable::PrintHelp to add "[options] <inputs>" explicitly?

[rupprecht]

I'm still against having a space in the string as the basis for interpreting it as a usage parameter. It's way too surprising.

include/llvm/Option/OptTable.h

229

What about including optional usage as a separate arg? e.g.

void PrintHelp(raw_ostream &OS, StringRef Name, StringRef Title,
               StringRef Usage="", ...) {
  OS << "USAGE: " << Name << (!Usage.empty() ? Usage : " [options] <inputs>") << '\n';
  ...
}

It's yet another method parameter, but there's zero magic involved.

[MaskRay]

include/llvm/Option/OptTable.h

229

This can be temporary. If https://reviews.llvm.org/D51109 looks good, I can add "[options] <inputs>" explicitly for all call sites.

[MaskRay]

Change the parameter to Usage

[MaskRay]

Update title

[MaskRay]

.

[rupprecht]

tools/llvm-objcopy/llvm-objcopy.cpp

900

extract this to a static const char[] like for strip below

[jakehehrlich]

In 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.

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.

tools/llvm-objcopy/llvm-objcopy.cpp

900

Maybe we should add "[options]" before "input"

Also, I think I'd prefer "<input>" to "input"

1042

I'd prefer "<inputs>" to "file..."

include/llvm/Option/OptTable.h

public:
   /// which could be parsed.
   InputArgList ParseArgs(ArrayRef<const char *> Args, unsigned &MissingArgIndex,
                          unsigned &MissingArgCount, unsigned FlagsToInclude = 0,
                          unsigned FlagsToExclude = 0) const;
 
   /// Render the help text for an option table.
   ///
   /// \param OS - The stream to write the help text to.
-  /// \param Name - The name to use in the usage line.
-  /// \param Title - The title to use in the usage line.
+  /// \param Usage - USAGE: Usage
+  /// \param Title - OVERVIEW: Title
   /// \param FlagsToInclude - If non-zero, only include options with any
   ///                         of these flags set.
   /// \param FlagsToExclude - Exclude options with any of these flags set.
   /// \param ShowAllAliases - If true, display all options including aliases
   ///                         that don't have help texts. By default, we display
   ///                         only options that are not hidden and have help
   ///                         texts.
-  void PrintHelp(raw_ostream &OS, const char *Name, const char *Title,
+  void PrintHelp(raw_ostream &OS, const char *Usage, const char *Title,
                  unsigned FlagsToInclude, unsigned FlagsToExclude,
                  bool ShowAllAliases) const;
 
-  void PrintHelp(raw_ostream &OS, const char *Name, const char *Title,
+  void PrintHelp(raw_ostream &OS, const char *Usage, const char *Title,
                  bool ShowHidden = false, bool ShowAllAliases = false) const;
 };
 
 } // end namespace opt
 
 } // end namespace llvm
 
 #endif // LLVM_OPTION_OPTTABLE_H

lib/Option/OptTable.cpp

static const char *getOptionHelpGroup(const OptTable &Opts, OptSpecifier Id) {
   // FIXME: Split out option groups.
   if (const char *GroupHelp = Opts.getOptionHelpText(GroupID))
     return GroupHelp;
 
   // Otherwise keep looking.
   return getOptionHelpGroup(Opts, GroupID);
 }
 
-void OptTable::PrintHelp(raw_ostream &OS, const char *Name, const char *Title,
+void OptTable::PrintHelp(raw_ostream &OS, const char *Usage, const char *Title,
                          bool ShowHidden, bool ShowAllAliases) const {
-  PrintHelp(OS, Name, Title, /*Include*/ 0, /*Exclude*/
+  PrintHelp(OS, Usage, Title, /*Include*/ 0, /*Exclude*/
             (ShowHidden ? 0 : HelpHidden), ShowAllAliases);
 }
 
-void OptTable::PrintHelp(raw_ostream &OS, const char *Name, const char *Title,
+void OptTable::PrintHelp(raw_ostream &OS, const char *Usage, const char *Title,
                          unsigned FlagsToInclude, unsigned FlagsToExclude,
                          bool ShowAllAliases) const {
-  OS << "OVERVIEW: " << Title << "\n";
-  OS << '\n';
-  OS << "USAGE: " << Name << " [options] <inputs>\n";
-  OS << '\n';
+  OS << "OVERVIEW: " << Title << "\n\n";
+  OS << "USAGE: " << Usage << "\n\n";
 
   // Render help text into a map of group-name to a list of (option, help)
   // pairs.
   std::map<std::string, std::vector<OptionInfo>> GroupedOptionHelp;
 
   for (unsigned Id = 1, e = getNumOptions() + 1; Id != e; ++Id) {
     // FIXME: Split out option groups.
     if (getOptionKind(Id) == Option::GroupClass)

test/tools/llvm-rc/helpmsg.test

 ; RUN: llvm-rc /h > %t1
 ; RUN: llvm-rc '/?' > %t2
 ; RUN: diff %t1 %t2
 ; RUN: FileCheck -input-file=%t1 %s
 
 ; CHECK:  OVERVIEW: Resource Converter
-; CHECK-DAG:  USAGE: rc [options] <inputs>
+; CHECK-DAG:  USAGE: rc [options] file...
 ; CHECK-DAG:  OPTIONS:
 ; CHECK-NEXT:    /?          Display this help and exit.
 ; CHECK-NEXT:    /C <value>  Set the codepage used for input strings.
 ; CHECK-NEXT:    /dry-run    Don't compile the input; only try to parse it.
 ; CHECK-NEXT:    /D <value>  Define a symbol for the C preprocessor.
 ; CHECK-NEXT:    /FO <value> Change the output file location.
 ; CHECK-NEXT:    /H          Display this help and exit.
 ; CHECK-NEXT:    /I <value>  Add an include path.
 ; CHECK-NEXT:    /LN <value> Set the default language name.
 ; CHECK-NEXT:    /L <value>  Set the default language identifier.
 ; CHECK-NEXT:    /N          Null-terminate all strings in the string table.
 ; CHECK-NEXT:    /U <value>  Undefine a symbol for the C preprocessor.
 ; CHECK-NEXT:    /V          Be verbose.
 ; CHECK-NEXT:    /X          Ignore 'include' variable.
 ; CHECK-NEXT:    /Y          Suppress warnings on duplicate resource IDs.

tools/llvm-cvtres/llvm-cvtres.cpp

int main(int Argc, const char **Argv) {
   InitLLVM X(Argc, Argv);
 
   CvtResOptTable T;
   unsigned MAI, MAC;
   ArrayRef<const char *> ArgsArr = makeArrayRef(Argv + 1, Argc - 1);
   opt::InputArgList InputArgs = T.ParseArgs(ArgsArr, MAI, MAC);
 
   if (InputArgs.hasArg(OPT_HELP)) {
-    T.PrintHelp(outs(), "cvtres", "Resource Converter", false);
+    T.PrintHelp(outs(), "cvtres [options] <inputs>", "Resource Converter", false);
     return 0;
   }
 
   bool Verbose = InputArgs.hasArg(OPT_VERBOSE);
 
   COFF::MachineTypes MachineType;
 
   if (InputArgs.hasArg(OPT_MACHINE)) {

tools/llvm-objcopy/llvm-objcopy.cpp

 // exit.
 static DriverConfig parseObjcopyOptions(ArrayRef<const char *> ArgsArr) {
   ObjcopyOptTable T;
   unsigned MissingArgumentIndex, MissingArgumentCount;
   llvm::opt::InputArgList InputArgs =
       T.ParseArgs(ArgsArr, MissingArgumentIndex, MissingArgumentCount);
 
   if (InputArgs.size() == 0) {
     T.PrintHelp(errs(), "llvm-objcopy <input> [ <output> ]", "objcopy tool");
     exit(1);
   }
 
   if (InputArgs.hasArg(OBJCOPY_help)) {
     T.PrintHelp(outs(), "llvm-objcopy <input> [ <output> ]", "objcopy tool");
     exit(0);
   }
 

 // help flag is set then ParseStripOptions will print the help messege and
 // exit.
 static DriverConfig parseStripOptions(ArrayRef<const char *> ArgsArr) {
   StripOptTable T;
   unsigned MissingArgumentIndex, MissingArgumentCount;
   llvm::opt::InputArgList InputArgs =
       T.ParseArgs(ArgsArr, MissingArgumentIndex, MissingArgumentCount);
 
+  static const char Usage[] = "llvm-strip [options] file...";
   if (InputArgs.size() == 0) {
-    T.PrintHelp(errs(), "llvm-strip", "strip tool");
+    T.PrintHelp(errs(), Usage, "strip tool");
     exit(1);
   }
 
   if (InputArgs.hasArg(STRIP_help)) {
-    T.PrintHelp(outs(), "llvm-strip", "strip tool");
+    T.PrintHelp(outs(), Usage, "strip tool");
     exit(0);
   }
 
   if (InputArgs.hasArg(STRIP_version)) {
     cl::PrintVersionMessage();
     exit(0);
   }
 

tools/llvm-rc/llvm-rc.cpp

int main(int Argc, const char **Argv) {
 
   RcOptTable T;
   unsigned MAI, MAC;
   ArrayRef<const char *> ArgsArr = makeArrayRef(Argv + 1, Argc - 1);
   opt::InputArgList InputArgs = T.ParseArgs(ArgsArr, MAI, MAC);
 
   // The tool prints nothing when invoked with no command-line arguments.
   if (InputArgs.hasArg(OPT_HELP)) {
-    T.PrintHelp(outs(), "rc", "Resource Converter", false);
+    T.PrintHelp(outs(), "rc [options] file...", "Resource Converter", false);
     return 0;
   }
 
   const bool BeVerbose = InputArgs.hasArg(OPT_VERBOSE);
 
   std::vector<std::string> InArgsInfo = InputArgs.getAllArgValues(OPT_INPUT);
   if (InArgsInfo.size() != 1) {
     fatalError("Exactly one input file should be provided.");

unittests/Option/OptionParsingTest.cpp

TEST(Option, OptionParsing) {
   EXPECT_EQ("2", Gs[1]);
 
   // Check the help text.
   std::string Help;
   raw_string_ostream RSO(Help);
   T.PrintHelp(RSO, "test", "title!");
   EXPECT_NE(std::string::npos, Help.find("-A"));
 
+  // Check usage line.
+  T.PrintHelp(RSO, "name [options] file...", "title!");
+  EXPECT_NE(std::string::npos, Help.find("USAGE: name [options] file...\n"));
+
   // Test aliases.
   auto Cs = AL.filtered(OPT_C);
   ASSERT_NE(Cs.begin(), Cs.end());
   EXPECT_EQ("desu", StringRef((*Cs.begin())->getValue()));
   ArgStringList ASL;
   (*Cs.begin())->render(AL, ASL);
   ASSERT_EQ(2u, ASL.size());
   EXPECT_EQ("-C", StringRef(ASL[0]));