[doc] Improve clang auto-generated help

[serge-sans-paille]

Summary

If no MetaVar is provided and an enumeration of values is set, document it in
the same way Python argparse does. Also add a proper unittest for help strings

This is a follow-up to 6d7317894f89cea29a2ecf7d86ac88adc40a5eaa

[serge-sans-paille]

@aaron.ballman : soft and gentle ping :-)

[aaron.ballman]

Generally looks reasonable to me, but it looks like Precommit CI caught some valid failures that need to be fixed:

Failed Tests (2):

Flang :: Driver/driver-help-hidden.f90
Flang :: Driver/driver-help.f90

llvm/lib/Option/OptTable.cpp

574–576

Use a Twine?

[serge-sans-paille]

Take review into account

[awarzynski]

Hi @serge-sans-paille, I just saw your comment on Discourse. Looking specifically at:

! HELP-FC1-NEXT: -std={c89,c90,iso9899:1990,iso9899:199409,gnu89,gnu90,c99,iso9899:1999,c9x,iso9899:199x,gnu99,gnu9x,c11,iso9899:2011,c1x,iso9899:201x,gnu11,gnu1x,c17,iso9899:2017,c18,iso9899:2018,gnu17,gnu18,c2x,gnu2x,c++98,c++03,gnu++98,gnu++03,c++11,c++0x,gnu++11,gnu++0x,c++14,c++1y,gnu++14,gnu++1y,c++17,c++1z,gnu++17,gnu++1z,c++20,c++2a,gnu++20,gnu++2a,c++2b,gnu++2b,cl1.0,cl,cl1.1,cl1.2,cl2.0,cl3.0,clc++1.0,clc++,clc++2021,CL,CL1.1,CL1.2,CL2.0,CL3.0,CLC++,CLC++1.0,CLC++2021,cuda,hip,hlsl,hlsl2015,hlsl2016,hlsl2017,hlsl2018,hlsl2021,hlsl202x,}

I think that listing the available options for -std= is helpful, but I'm not sure whether clang -help is the right place. It's already quite cluttered and impossible to navigate without piping to e.g. grep. In the past, we tried using DocBrief in Options.td for longer option comments. That wouldn't change the contents of clang -help, but IIRC, would be included in https://clang.llvm.org/docs/ClangCommandLineReference.html. In general, I think that it would be great to re-factor and organise clang -help a bit, but that would be a bigger job.

On a different note, we should definitely avoiding making flang-new display various C or C++ standards as available options for -std=<val>. Basically, -std=c++98 (and other similar options) are not supported by flang-new and should not be printed in flang-new -help.

HTH,
Andrzej

[serge-sans-paille]

Thanks for jumping in! This looks like a design error to me. The Values field is already used for code completion, so the behavior is already flawed... Whould it make sense to have a common file for shared options, and then a clang-specific and a flang specifc option for options with different values / meaning etc ?

[awarzynski]

In D122654#3454126, @serge-sans-paille wrote:

Thanks for jumping in!

Np! And apologies for going radio silent for so long, completely lost track of time!

This looks like a design error to me. The Values field is already used for code completion, so the behavior is already flawed...

Hm, why wouldn't you want to auto-complete with possible values too? For example, -std=c<tab> will only work if possible values are also used in auto-completion.

Whould it make sense to have a common file for shared options, and then a clang-specific and a flang specifc option for options with different values / meaning etc ?

Yes! :) Many options do/will have identical meaning in both frontends. But I agree that there should be a dedicated file for every driver:

• clang (Clang's compiler driver)
• clang -cc1 (Clang's frontend driver)
• clang -cc1as (Clang's integrated assembler driver)
• flang (Flang's compiler driver)
• flang-new -fc1 (Flang's frontend driver)
• <anything that I've missed>

Perhaps this is more extreme than what you had in mind? Shared options (with semantics that don't differ between drivers) could be kept in some common *.td file.

In general, IMHO Options.td is a good example of code re-use not being such a good thing :) @ekieri made some nice improvements there recently: https://reviews.llvm.org/D123070. And I discussed this briefly in the past: https://lists.llvm.org/pipermail/cfe-dev/2020-July/066393.html too.

llvm/include/llvm/Option/OptTable.h

public:
   }
 
   /// Get the meta-variable name to use when describing
   /// this options values in the help text.
   const char *getOptionMetaVar(OptSpecifier id) const {
     return getInfo(id).MetaVar;
   }
 
+  /// Get the values this option can take.
+  const char *getOptionValues(OptSpecifier id) const {
+    return getInfo(id).Values;
+  }
+
   /// Specify the environment variable where initial options should be read.
   void setInitialOptionsFromEnvironment(const char *E) { EnvVar = E; }
 
   /// Support grouped short options. e.g. -ab represents -a -b.
   void setGroupedShortOptions(bool Value) { GroupedShortOptions = Value; }
 
   /// Find possible value for given flags. This is used for shell
   /// autocompletion.

llvm/lib/Option/OptTable.cpp

static std::string getOptionHelpName(const OptTable &Opts, OptSpecifier Id) {
   case Option::SeparateClass: case Option::JoinedOrSeparateClass:
   case Option::RemainingArgsClass: case Option::RemainingArgsJoinedClass:
     Name += ' ';
     LLVM_FALLTHROUGH;
   case Option::JoinedClass: case Option::CommaJoinedClass:
   case Option::JoinedAndSeparateClass:
     if (const char *MetaVarName = Opts.getOptionMetaVar(Id))
       Name += MetaVarName;
-    else
+    else if (const char *Values = Opts.getOptionValues(Id)) {
+      Name += "{";
+      Name += Values;
+      Name += "}";
+    } else
       Name += "<value>";
     break;
   }
 
   return Name;
 }
 
 namespace {

llvm/unittests/Option/OptionParsingTest.cpp

TEST(Option, OptionParsing) {
   std::vector<std::string> Gs = AL.getAllArgValues(OPT_G);
   EXPECT_EQ("chuu", Gs[0]);
   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"));
+  std::string RefHelp = "OVERVIEW: title!\n"
+                        "\n"
+                        "USAGE: test\n"
+                        "\n"
+                        "OPTIONS:\n"
+                        "  -A                    The A option\n"
+                        "  --blarn               The blarn option\n"
+                        "  -blorp                The blorp option\n"
+                        "  -BB                   The B option\n"
+                        "  /cramb:CRAMB          The cramb option\n"
+                        "  -C C                  The C option\n"
+                        "  /C C                  The C option\n"
+                        "  -doopf1               The doopf1 option\n"
+                        "  -doopf2               The doopf2 option\n"
+                        "  -DD                   The D option\n"
+                        "  --ermghERMGH          The ermgh option\n"
+                        "  --fjormp              The fjormp option\n"
+                        "  -F F                  The F option\n"
+                        "  -GG                   The G option\n"
+                        "  --with-values={1,2,3} A valuable option\n";
+  EXPECT_EQ(Help, RefHelp);
 
   // 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());

llvm/unittests/Option/Opts.td

 
 def Blorp : Flag<["-", "--"], "blorp">, HelpText<"The blorp option">, Flags<[OptFlag1]>;
 def Blarn : Flag<["--", "-"], "blarn">, HelpText<"The blarn option">, Flags<[OptFlag1]>;
 def Cramb : Joined<["/"], "cramb:">, HelpText<"The cramb option">, MetaVarName<"CRAMB">, Flags<[OptFlag1]>;
 def Doopf1 : Flag<["-"], "doopf1">, HelpText<"The doopf1 option">, Flags<[OptFlag1]>;
 def Doopf2 : Flag<["-"], "doopf2">, HelpText<"The doopf2 option">, Flags<[OptFlag2]>;
 def Ermgh : Joined<["--"], "ermgh">, HelpText<"The ermgh option">, MetaVarName<"ERMGH">, Flags<[OptFlag1]>;
 def Fjormp : Flag<["--"], "fjormp">, HelpText<"The fjormp option">, Flags<[OptFlag1]>;
+def WithValues : Joined<["--"], "with-values=">, HelpText<"A valuable option">, Flags<[OptFlag1]>, Values<"1,2,3">;
 
 def Glorrmp_eq : Flag<["--"], "glorrmp=">;
 
 def Blurmpq : Flag<["--"], "blurmp">;
 def Blurmpq_eq : Flag<["--"], "blurmp=">;
 
 def DashDash : Option<["--"], "", KIND_REMAINING_ARGS>;