[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

230

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

230

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 ↗

(On Diff #168911)

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 ↗

(On Diff #168911)

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

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

1042 ↗

(On Diff #168911)

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 NameOrUsage - The usage line. If without space, "[options] <inputs>"
+  ///                      will be appended.
   /// \param Title - The title to use in the usage line.
   /// \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 *NameOrUsage, 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 *NameOrUsage, 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 *NameOrUsage, const char *Title,
                          bool ShowHidden, bool ShowAllAliases) const {
-  PrintHelp(OS, Name, Title, /*Include*/ 0, /*Exclude*/
+  PrintHelp(OS, NameOrUsage, 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 *NameOrUsage, 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 << "USAGE: " << NameOrUsage;
+  if (!strchr(NameOrUsage, ' '))
+    OS << " [options] <inputs>";
+  OS << "\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)

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", "title!");
+  EXPECT_NE(std::string::npos, Help.find("USAGE: name [options] <inputs>\n"));
+  T.PrintHelp(RSO, "name <input>", "title!");
+  EXPECT_NE(std::string::npos, Help.find("USAGE: name <input>\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]));