[CommandLine] Change help output to prefix long options with `--` instead of `-`. NFC . Part 3 of 5

[hintonda]

Summary

By default, parseCommandLineOptions() will accept either a
- or -- prefix for long options -- options with names longer than
a single character.

While this change does not affect behavior, it will be helpful with a
subsequent change that requires long options use the -- prefix.

[hintonda]

• Fix dashes for help on error.

[hintonda]

• Fix dashes in error messages and in tests.

[thopre]

llvm/lib/Support/CommandLine.cpp

95

Any reason why not take a StringRef of the option to compute the prefix of? That would put the .size() gymnastic in one place instead of many.

101

Likewise.

[hintonda]

llvm/lib/Support/CommandLine.cpp

95

Actually, that's what I originally had, but since it was only a few places, I changed it to take the length. Now that I see it's used all over, I'll probably switch it back as you suggest.

[hintonda]

• Refactor and fix additional dashes in error messages and tests.

[thopre]

That's much better thank you. I like the stream shift operator overload, nice touch. LGTM but give it a day for other to give feedback.

[thakis]

I happened to see this go by. Is there an explanation of the overall goal somewhere? In general, requiring -- for long flags sounds like a great change to me, but there are a few exceptions: For example. lld-link should keep accepting long flags with a single dash for link.exe compatibility.

[hintonda]

In D61269#1490176, @thakis wrote:

I happened to see this go by. Is there an explanation of the overall goal somewhere? In general, requiring -- for long flags sounds like a great change to me, but there are a few exceptions: For example. lld-link should keep accepting long flags with a single dash for link.exe compatibility.

The 5th and final patch, D61294, "[CommandLine] Add long option flag for cl::ParseCommandLineOptions . Part 5 of 5", adds a flag that requires long options use the --. It's an opt-in strategy, so only tools that want the GNU getopt_long behavior would want to set it. Otherwise, everything works the way it does currently, with the exception of help printing -- for long options.

As noted in D61294, these changes were motivated by this discussion: http://lists.llvm.org/pipermail/llvm-dev/2019-April/131786.html

Of course, this doesn't change tblgen generated options which are a different animal.

[thakis]

Thanks for the context. Sounds great :)

llvm/lib/Support/CommandLine.cpp

 void parser<char>::anchor() {}
 
 //===----------------------------------------------------------------------===//
 
 namespace {
 
 class CommandLineParser {
 public:
   // Globals for name and overview of program.  Program name is not a string to
   // avoid static ctor/dtor issues.
   std::string ProgramName;
   StringRef ProgramOverview;
 
   // This collects additional help to be printed.
   std::vector<StringRef> MoreHelp;
 
   // This collects Options added with the cl::DefaultOption flag. Since they can
   // be overridden, they are not added to the appropriate SubCommands until
   // ParseCommandLineOptions actually runs.
   SmallVector<Option*, 4> DefaultOptions;
 
   // This collects the different option categories that have been registered.
   SmallPtrSet<OptionCategory *, 16> RegisteredOptionCategories;

 //
 static StringRef getValueStr(const Option &O, StringRef DefaultMsg) {
   if (O.ValueStr.empty())
     return DefaultMsg;
   return O.ValueStr;
 }
 
 static StringRef ArgPrefix = "  -";
+static StringRef ArgPrefixLong = "  --";
 static StringRef ArgHelpPrefix = " - ";
-static size_t ArgPrefixesSize = ArgPrefix.size() + ArgHelpPrefix.size();
+
+static size_t argPrefixesSize(size_t len) {
+  if (len == 1)
+    return ArgPrefix.size() + ArgHelpPrefix.size();
+  return ArgPrefixLong.size() + ArgHelpPrefix.size();
+}
+
+static StringRef argPrefix(size_t len) {
+  if (len == 1)
+    return ArgPrefix;
+  return ArgPrefixLong;
+}
 
 //===----------------------------------------------------------------------===//
 // cl::alias class implementation
 //
 
 // Return the width of the option tag for printing...
-size_t alias::getOptionWidth() const { return ArgStr.size() + ArgPrefixesSize; }
+size_t alias::getOptionWidth() const {
+  return ArgStr.size() + argPrefixesSize(ArgStr.size());
+}
 
 void Option::printHelpStr(StringRef HelpStr, size_t Indent,
                           size_t FirstLineIndentedBy) {
   assert(Indent >= FirstLineIndentedBy);
   std::pair<StringRef, StringRef> Split = HelpStr.split('\n');
   outs().indent(Indent - FirstLineIndentedBy)
       << ArgHelpPrefix << Split.first << "\n";
   while (!Split.second.empty()) {
     Split = Split.second.split('\n');
     outs().indent(Indent) << Split.first << "\n";
   }
 }
 
 // Print out the option for the alias.
 void alias::printOptionInfo(size_t GlobalWidth) const {
-  outs() << ArgPrefix << ArgStr;
-  printHelpStr(HelpStr, GlobalWidth, ArgStr.size() + ArgPrefixesSize);
+  outs() << argPrefix(ArgStr.size()) << ArgStr;
+  printHelpStr(HelpStr, GlobalWidth,
+               ArgStr.size() + argPrefixesSize(ArgStr.size()));
 }
 
 //===----------------------------------------------------------------------===//
 // Parser Implementation code...
 //
 
 // basic_parser implementation
 //
 
 // Return the width of the option tag for printing...
 size_t basic_parser_impl::getOptionWidth(const Option &O) const {
   size_t Len = O.ArgStr.size();
   auto ValName = getValueName();
   if (!ValName.empty()) {
     size_t FormattingLen = 3;
     if (O.getMiscFlags() & PositionalEatsArgs)
       FormattingLen = 6;
     Len += getValueStr(O, ValName).size() + FormattingLen;
   }
 
-  return Len + ArgPrefixesSize;
+  return Len + argPrefixesSize(O.ArgStr.size());
 }
 
 // printOptionInfo - Print out information about this option.  The
 // to-be-maintained width is specified.
 //
 void basic_parser_impl::printOptionInfo(const Option &O,
                                         size_t GlobalWidth) const {
-  outs() << ArgPrefix << O.ArgStr;
+  outs() << argPrefix(O.ArgStr.size()) << O.ArgStr;
 
   auto ValName = getValueName();
   if (!ValName.empty()) {
     if (O.getMiscFlags() & PositionalEatsArgs) {
       outs() << " <" << getValueStr(O, ValName) << ">...";
     } else {
       outs() << "=<" << getValueStr(O, ValName) << '>';
     }
   }
 
   Option::printHelpStr(O.HelpStr, GlobalWidth, getOptionWidth(O));
 }
 
 void basic_parser_impl::printOptionName(const Option &O,
                                         size_t GlobalWidth) const {
-  outs() << ArgPrefix << O.ArgStr;
+  outs() << argPrefix(O.ArgStr.size()) << O.ArgStr;
   outs().indent(GlobalWidth - O.ArgStr.size());
 }
 
 // parser<bool> implementation
 //
 bool parser<bool>::parse(Option &O, StringRef ArgName, StringRef Arg,
                          bool &Value) {
   if (Arg == "" || Arg == "true" || Arg == "TRUE" || Arg == "True" ||
static bool shouldPrintOption(StringRef Name, StringRef Description,
                               const Option &O) {
   return O.getValueExpectedFlag() != ValueOptional || !Name.empty() ||
          !Description.empty();
 }
 
 // Return the width of the option tag for printing...
 size_t generic_parser_base::getOptionWidth(const Option &O) const {
   if (O.hasArgStr()) {
-    size_t Size = O.ArgStr.size() + ArgPrefixesSize + EqValue.size();
+    size_t Size =
+        O.ArgStr.size() + argPrefixesSize(O.ArgStr.size()) + EqValue.size();
     for (unsigned i = 0, e = getNumOptions(); i != e; ++i) {
       StringRef Name = getOption(i);
       if (!shouldPrintOption(Name, getDescription(i), O))
         continue;
       size_t NameSize = Name.empty() ? EmptyOption.size() : Name.size();
       Size = std::max(Size, NameSize + OptionPrefixesSize);
     }
     return Size;

 void generic_parser_base::printOptionInfo(const Option &O,
                                           size_t GlobalWidth) const {
   if (O.hasArgStr()) {
     // When the value is optional, first print a line just describing the
     // option without values.
     if (O.getValueExpectedFlag() == ValueOptional) {
       for (unsigned i = 0, e = getNumOptions(); i != e; ++i) {
         if (getOption(i).empty()) {
-          outs() << ArgPrefix << O.ArgStr;
+          outs() << argPrefix(O.ArgStr.size()) << O.ArgStr;
           Option::printHelpStr(O.HelpStr, GlobalWidth,
-                               O.ArgStr.size() + ArgPrefixesSize);
+                               O.ArgStr.size() +
+                                   argPrefixesSize(O.ArgStr.size()));
           break;
         }
       }
     }
 
-    outs() << ArgPrefix << O.ArgStr << EqValue;
+    outs() << argPrefix(O.ArgStr.size()) << O.ArgStr << EqValue;
     Option::printHelpStr(O.HelpStr, GlobalWidth,
-                         O.ArgStr.size() + EqValue.size() + ArgPrefixesSize);
+                         O.ArgStr.size() + EqValue.size() +
+                             argPrefixesSize(O.ArgStr.size()));
     for (unsigned i = 0, e = getNumOptions(); i != e; ++i) {
       StringRef OptionName = getOption(i);
       StringRef Description = getDescription(i);
       if (!shouldPrintOption(OptionName, Description, O))
         continue;
       assert(GlobalWidth >= OptionName.size() + OptionPrefixesSize);
       size_t NumSpaces = GlobalWidth - OptionName.size() - OptionPrefixesSize;
       outs() << OptionPrefix << OptionName;