[OptTable] Refine how `printHelp` treats empty help texts

[awarzynski]

Summary

Currently, printHelp behaves differently for options that:

• do not define HelpText (such options _are not_ printed), and
• define its HelpText as HelpText<""> (such options _are_ printed).

In practice, both approaches lead to no help text and printHelp should
treat them consistently. This patch addresses that by making
printHelpt check the length of the help text to be printed.

[awarzynski]

With this change, llvm/test/tools/llvm-cvtres/help.tes is failing. That's because the help text in llvm-cvtres relies on the old behavior. The corresponding *.td file is tiny and perhaps @thakis or @mstorsjo could suggest how to update it?

[jansvoboda11]

This LGTM provided we figure out what to about llvm-cvtres.

[mstorsjo]

In D107557#2928167, @awarzynski wrote:

With this change, llvm/test/tools/llvm-cvtres/help.tes is failing. That's because the help text in llvm-cvtres relies on the old behavior. The corresponding *.td file is tiny and perhaps @thakis or @mstorsjo could suggest how to update it?

Hmm, that's a good question. Most of the options that are listed aren't even hooked up in llvm-cvtres (and the tool it impersonates, MS cvtres.exe, also just lists the options by name without any help text in its help printing).

I guess we could add a help text for the options that we have implemented at least, and then omit the other ones. Or do @thakis or @amccarth have a strong opinion on it?

[amccarth]

In D107557#2929544, @mstorsjo wrote:

Hmm, that's a good question. Most of the options that are listed aren't even hooked up in llvm-cvtres (and the tool it impersonates, MS cvtres.exe, also just lists the options by name without any help text in its help printing).

I guess we could add a help text for the options that we have implemented at least, and then omit the other ones. Or do @thakis or @amccarth have a strong opinion on it?

I don't have a strong opinion either way. If a tool accepts an option, I'd prefer to see it listed. If these options don't have help text because the tool is ignoring the option (but allowing it for compatibility), then I'd suggest we add some help text that says something to that effect, like "not implemented."

With clang-cl /?, I see /Og as "no effect". There are also a few described as "Deprecated, <original purpose>".

@thakis may have stronger opinions on this than I do.

[awarzynski]

Add help text to all options in llvm-cvtres

I've tried to come up with something sensible for every option. As suggested by @amccarth, options that are not implemented get "Not implemented".

Thank you for reviewing!

[awarzynski]

Hi, I believe that I've addressed all of your comments and would like to merge this. I will be away starting from next week and want to make sure that this doesn't become stale.

Please leave a comment if you feel that this is not ready. Otherwise, I will merge this before the end of the week. Thank you for all your comments!

[mstorsjo]

FWIW, the change looks sensible to me.

flang/test/Driver/driver-help-hidden.f90

 ! CHECK-NEXT: Use <value> as character line width in fixed mode
 ! CHECK-NEXT: -ffree-form            Process source files in free form
 ! CHECK-NEXT: -fimplicit-none        No implicit typing allowed unless overridden by IMPLICIT statements
 ! CHECK-NEXT: -finput-charset=<value> Specify the default character set for source files
 ! CHECK-NEXT: -fintrinsic-modules-path <dir>
 ! CHECK-NEXT:                        Specify where to find the compiled intrinsic modules
 ! CHECK-NEXT: -flarge-sizes          Use INTEGER(KIND=8) for the result type in size-related intrinsics
 ! CHECK-NEXT: -flogical-abbreviations Enable logical abbreviations
-! CHECK-NEXT: -fno-backslash
 ! CHECK-NEXT: -fno-color-diagnostics  Disable colors in diagnostics
-! CHECK-NEXT: -fno-implicit-none
-! CHECK-NEXT: -fno-logical-abbreviations
-! CHECK-NEXT: {{[[:space:]]$}}
-! CHECK-NEXT: -fno-xor-operator
 ! CHECK-NEXT: -fopenacc              Enable OpenACC
 ! CHECK-NEXT: -fopenmp               Parse OpenMP pragmas and generate parallel code.
 ! CHECK-NEXT: -fxor-operator         Enable .XOR. as a synonym of .NEQV.
 ! CHECK-NEXT: -help     Display available options
 ! CHECK-NEXT: -I <dir>               Add directory to the end of the list of include search paths
 ! CHECK-NEXT: -module-dir <dir>      Put MODULE files in <dir>
 ! CHECK-NEXT: -nocpp                 Disable predefined and command line preprocessor macros
 ! CHECK-NEXT: -o <file> Write output to <file>

flang/test/Driver/driver-help.f90

 ! HELP-NEXT: Use <value> as character line width in fixed mode
 ! HELP-NEXT: -ffree-form            Process source files in free form
 ! HELP-NEXT: -fimplicit-none        No implicit typing allowed unless overridden by IMPLICIT statements
 ! HELP-NEXT: -finput-charset=<value> Specify the default character set for source files
 ! HELP-NEXT: -fintrinsic-modules-path <dir>
 ! HELP-NEXT:                        Specify where to find the compiled intrinsic modules
 ! HELP-NEXT: -flarge-sizes          Use INTEGER(KIND=8) for the result type in size-related intrinsics
 ! HELP-NEXT: -flogical-abbreviations Enable logical abbreviations
-! HELP-NEXT: -fno-backslash
 ! HELP-NEXT: -fno-color-diagnostics  Disable colors in diagnostics
-! HELP-NEXT: -fno-implicit-none
-! HELP-NEXT: -fno-logical-abbreviations
-! HELP-NEXT: {{[[:space:]]$}}
-! HELP-NEXT: -fno-xor-operator
 ! HELP-NEXT: -fopenacc              Enable OpenACC
 ! HELP-NEXT: -fopenmp               Parse OpenMP pragmas and generate parallel code.
 ! HELP-NEXT: -fxor-operator         Enable .XOR. as a synonym of .NEQV.
 ! HELP-NEXT: -help                  Display available options
 ! HELP-NEXT: -I <dir>               Add directory to the end of the list of include search paths
 ! HELP-NEXT: -module-dir <dir>      Put MODULE files in <dir>
 ! HELP-NEXT: -nocpp                 Disable predefined and command line preprocessor macros
 ! HELP-NEXT: -o <file>              Write output to <file>

 ! HELP-FC1-EMPTY:
 ! HELP-FC1-NEXT:OPTIONS:
 ! HELP-FC1-NEXT: -cpp                   Enable predefined and command line preprocessor macros
 ! HELP-FC1-NEXT: -D <macro>=<value>     Define <macro> to <value> (or 1 if <value> omitted)
 ! HELP-FC1-NEXT: -emit-obj Emit native object files
 ! HELP-FC1-NEXT: -E                     Only run the preprocessor
 ! HELP-FC1-NEXT: -falternative-parameter-statement
 ! HELP-FC1-NEXT: Enable the old style PARAMETER statement
-! HELP-FC1-NEXT: -fanalyzed-objects-for-unparse
-! HELP-FC1-NEXT: {{[[:space:]]$}}
 ! HELP-FC1-NEXT: -fbackslash            Specify that backslash in string introduces an escape character
 ! HELP-FC1-NEXT: -fdebug-dump-all       Dump symbols and the parse tree after the semantic checks
 ! HELP-FC1-NEXT: -fdebug-dump-parse-tree-no-sema
 ! HELP-FC1-NEXT:                        Dump the parse tree (skips the semantic checks)
 ! HELP-FC1-NEXT: -fdebug-dump-parse-tree Dump the parse tree
 ! HELP-FC1-NEXT: -fdebug-dump-parsing-log
 ! HELP-FC1-NEXT:                   Run instrumented parse and dump the parsing log
 ! HELP-FC1-NEXT: -fdebug-dump-provenance Dump provenance

 ! HELP-FC1-NEXT: -fimplicit-none        No implicit typing allowed unless overridden by IMPLICIT statements
 ! HELP-FC1-NEXT: -finput-charset=<value> Specify the default character set for source files
 ! HELP-FC1-NEXT: -fintrinsic-modules-path <dir>
 ! HELP-FC1-NEXT:                        Specify where to find the compiled intrinsic modules
 ! HELP-FC1-NEXT: -flarge-sizes          Use INTEGER(KIND=8) for the result type in size-related intrinsics
 ! HELP-FC1-NEXT: -flogical-abbreviations Enable logical abbreviations
 ! HELP-FC1-NEXT: -fno-analyzed-objects-for-unparse
 ! HELP-FC1-NEXT:                        Do not use the analyzed objects when unparsing
-! HELP-FC1-NEXT: -fno-backslash
-! HELP-FC1-NEXT: -fno-implicit-none
-! HELP-FC1-NEXT: -fno-logical-abbreviations
-! HELP-FC1-NEXT: {{[[:space:]]$}}
 ! HELP-FC1-NEXT: -fno-reformat          Dump the cooked character stream in -E mode
-! HELP-FC1-NEXT: -fno-xor-operator
 ! HELP-FC1-NEXT: -fopenacc              Enable OpenACC
 ! HELP-FC1-NEXT: -fopenmp               Parse OpenMP pragmas and generate parallel code.
 ! HELP-FC1-NEXT: -fxor-operator         Enable .XOR. as a synonym of .NEQV.
 ! HELP-FC1-NEXT: -help                  Display available options
 ! HELP-FC1-NEXT: -init-only             Only execute frontend initialization
 ! HELP-FC1-NEXT: -I <dir>               Add directory to the end of the list of include search paths
 ! HELP-FC1-NEXT: -load <dsopath>        Load the named plugin (dynamic shared object)
 ! HELP-FC1-NEXT: -module-dir <dir>      Put MODULE files in <dir>

llvm/lib/Option/OptTable.cpp

for (unsigned Id = 1, e = getNumOptions() + 1; Id != e; ++Id) {
     // option instead.
     const char *HelpText = getOptionHelpText(Id);
     if (!HelpText && ShowAllAliases) {
       const Option Alias = getOption(Id).getAlias();
       if (Alias.isValid())
         HelpText = getOptionHelpText(Alias.getID());
     }
 
-    if (HelpText) {
+    if (HelpText && (strlen(HelpText) != 0)) {
       const char *HelpGroup = getOptionHelpGroup(*this, Id);
       const std::string &OptName = getOptionHelpName(*this, Id);
       GroupedOptionHelp[HelpGroup].push_back({OptName, HelpText});
     }
   }
 
   for (auto& OptionGroup : GroupedOptionHelp) {
     if (OptionGroup.first != GroupedOptionHelp.begin()->first)
       OS << "\n";
     PrintHelpOptionList(OS, OptionGroup.first, OptionGroup.second);
   }
 
   OS.flush();
 }

llvm/test/tools/llvm-cvtres/help.test

 ; RUN: llvm-cvtres /h > %t
 ; RUN: FileCheck -input-file=%t %s -check-prefix=HELP_TEST
 
 ; HELP_TEST:    OVERVIEW: Resource Converter
 ; HELP_TEST-DAG:  USAGE: llvm-cvtres [options] file...
 ; HELP_TEST-DAG:  OPTIONS:
-; HELP_TEST-NEXT:   /DEFINE:symbol 
-; HELP_TEST-NEXT:   /FOLDDUPS:     
+; HELP_TEST-NEXT:   /DEFINE:symbol Not implemented
+; HELP_TEST-NEXT:   /FOLDDUPS: Not implemented
+; HELP_TEST-NEXT:   /HELP Display available options
 ; HELP_TEST-NEXT:   /MACHINE:{ARM|ARM64|EBC|IA64|X64|X86}
-; HELP_TEST-DAG:    /NOLOGO        
-; HELP_TEST-NEXT:   /OUT:filename  
-; HELP_TEST-NEXT:   /READONLY      
+; HELP_TEST-NEXT:   Machine architecture
+; HELP_TEST-DAG:    /NOLOGO Not implemented
+; HELP_TEST-NEXT:   /OUT:filename Output file
+; HELP_TEST-NEXT:   /READONLY Not implemented
 ; HELP_TEST-NEXT:   /TIMESTAMP:<value> Timestamp for coff header, defaults to current time
-; HELP_TEST-NEXT:   /VERBOSE
+; HELP_TEST-NEXT:   /VERBOSE Use verbose output

llvm/tools/llvm-cvtres/Opts.td

 include "llvm/Option/OptParser.td"
 
 // All the switches can be preceded by either '/' or '-'.
 
-def DEFINE : Joined<["/", "-"], "DEFINE:">, HelpText<"">, MetaVarName<"symbol">;
-def FOLDDUPS : Flag<["/", "-"], "FOLDDUPS:">, HelpText<"">;
-def MACHINE : Joined<["/", "-"], "MACHINE:">, HelpText<"">, MetaVarName<"{ARM|ARM64|EBC|IA64|X64|X86}">;
-def NOLOGO : Flag<["/", "-"], "NOLOGO">, HelpText<"">;
-def OUT : Joined<["/", "-"], "OUT:">, HelpText<"">, MetaVarName<"filename">;
-def READONLY : Flag<["/", "-"], "READONLY">, HelpText<"">;
-def VERBOSE : Flag<["/", "-"], "VERBOSE">, HelpText<"">;
-def HELP : Flag<["/", "-"], "HELP">;
+def DEFINE : Joined<["/", "-"], "DEFINE:">, HelpText<"Not implemented">, MetaVarName<"symbol">;
+def FOLDDUPS : Flag<["/", "-"], "FOLDDUPS:">, HelpText<"Not implemented">;
+def MACHINE : Joined<["/", "-"], "MACHINE:">, HelpText<"Machine architecture">, MetaVarName<"{ARM|ARM64|EBC|IA64|X64|X86}">;
+def NOLOGO : Flag<["/", "-"], "NOLOGO">, HelpText<"Not implemented">;
+def OUT : Joined<["/", "-"], "OUT:">, HelpText<"Output file">, MetaVarName<"filename">;
+def READONLY : Flag<["/", "-"], "READONLY">, HelpText<"Not implemented">;
+def VERBOSE : Flag<["/", "-"], "VERBOSE">, HelpText<"Use verbose output">;
+def HELP : Flag<["/", "-"], "HELP">, HelpText<"Display available options">;
 def H : Flag<["/", "-"], "H">, Alias<HELP>;
 def HELP_Q : Flag<["/?", "-?"], "">, Alias<HELP>;
 
 // Extensions.
 
 def TIMESTAMP : Joined<["/", "-"], "TIMESTAMP:">,
     HelpText<"Timestamp for coff header, defaults to current time">;