[OptTable] Support grouped short options

[MaskRay]

Summary

POSIX.1-2017 12.2 Utility Syntax Guidelines, Guideline 5 says:

One or more options without option-arguments, followed by at most one option that takes an option-argument, should be accepted when grouped behind one '-' delimiter.

i.e. -abc represents -a -b -c. The grouped short options are very common. Many
utilities extend the syntax by allowing (an option with an argument) following a
sequence of short options.

This patch adds the support to OptTable, similar to cl::Group for CommandLine
(D58711). llvm-symbolizer will use the feature (D83530). CommandLine is exotic
in some aspects. OptTable is preferred if the user wants to get rid of the
behaviors.

• cl::opt<bool> i(...) can be disabled via -i=false or -i=0, which is different from conventional --no-i.
• Handling --foo & --no-foo requires a comparison of argument positions, which is a bit clumsy in user code.

OptTable::parseOneArg (non-const reference InputArgList) is added along with
ParseOneArg (const ArgList &). The duplicate does not look great at first
glance. However, The implementation can be simpler if ArgList is mutable.
(ParseOneArg is used by clang-cl (FlagsToInclude/FlagsToExclude) and lld COFF
(case-insensitive). Adding grouped short options can make the function even more
complex.)

The implementation allows a long option following a group of short options. We
probably should refine the code to disallow this in the future. Allowing this
seems benign for now.

[jhenderson]

llvm/include/llvm/Option/OptTable.h

200–201

"return" -> "\return"
"Index" -> "\p Index"
"Args" -> "\p Args".

(i.e. use the doxygen style used elsewhere in this file)

llvm/include/llvm/Option/Option.h

208–216

This comment probably wants updating given the API change.

llvm/lib/Option/OptTable.cpp

333

I've not looked too much in depth yet at this, but this function seems very similar to the one immediately below? Can the two somehow be combined?

350–351

I feel like this would be more clearly written as:

unsigned ArgSize = matchOption(Start, Str, IgnoreCase);
if (!ArgSize)

llvm/unittests/Option/OptionParsingTest.cpp

359–363

If we probably should disallow this, why don't we?

[grimar]

llvm/lib/Option/OptTable.cpp

472–476

What happens when GroupedShortOptions is true and FlagsToInclude/FlagsToExclude are present?

474

This looks strange. We should not have 2 versions of parseOneArg/ParseOneArg that have different naming styles (lower-case vs upper-case) of the same name.

Also, their signatures (given that InputArgList inherits from ArgList) are very close:

Arg *ParseOneArg(const ArgList &Args, unsigned &Index,
                 unsigned FlagsToInclude = 0,
                 unsigned FlagsToExclude = 0) const;

Arg *parseOneArg(InputArgList &Args, unsigned &Index) const;

And it is probably too easy to call one instead of another.
So it looks like either they should be combined, or, if it is not possible, probably the new parseOneArg should be renamed to something else?

[MaskRay]

Address comments

[ikudrin]

llvm/include/llvm/Option/ArgList.h

414

Add an empty line before the method.

llvm/include/llvm/Option/OptTable.h

62

Wouldn't it be better if the grouping is a property of options themselves rather then a flag in the parser? That would allow better tuning of options.

llvm/lib/Option/OptTable.cpp

358

The magic value 2 needs an explanatory comment.

358

The code can't get here if GroupedShortOptions is false, no? Though, I'd prefer the two parsing methods to be merged.

[MaskRay]

llvm/lib/Option/OptTable.cpp

333

Answered below.

472–476

There is no such use case.

FlagsToInclude/FlagsToExclude is only used by clang-cl. clang does not use grouped short options.

474

I have mentioned in the description that having two methods is not great but isn't a bad choice.

... The implementation can be simpler if ArgList is mutable.

(ParseOneArg is used by clang-cl (FlagsToInclude/FlagsToExclude) and lld COFF
(case-insensitive). Adding grouped short options can make the function even more
complex.)

ParseOneArg requires const ArgList &Args in some clang call sites. The interface has been made complex by clang-cl and lld COFF (case-insensitive parsing). Having a duplicate parseOneArg actually isolates the complexity without adding too many lines.

In the future, we may consider dropping -long-option support from this overload. This may allow us to optimize/simplify the function. Combining it with ParseShortOptions may add a large switch inside a for loop, which may not be desired.

llvm/unittests/Option/OptionParsingTest.cpp

359–363

This syntax only make sense when there is one dash. If we drop -long-option support, this will be naturally disallowed.

[MaskRay]

Address comments. Improve tests

llvm/include/llvm/Option/OptTable.h

62

Grouped short options is all or nothing. I think it is hardly the case that a utility supports some short options being grouped. This is a compromised design in CommandLine AFAICT.

llvm/lib/Option/OptTable.cpp

358

Removed GroupedShortOptions. I have explained that some call sites require refactoring (due to const reference) if we want to merge parseOneOption & ParseOneOption.

[MaskRay]

I have mentioned why I chose for a new parseOneArg rather than extending the existing ParseOneArg. I'll make more comments here:

On the interface side:

• ParseOneArg needs to work with a const reference and a DerivedArgList (only used in clang driver). clang driver has unique requirement that some features require to parse on argument on the fly.
• parseOneArg needs to work with a mutable ArgList. Since we don't need complex stuff in clang, I simply make the parameter InputArgList &. The function is only called/driven by OptTable::ParseArgs. The instance is mutable - so there is no point making InputArgList & const. It would made several called functions complex.

On the feature side:

• (a) ParseOneArg needs to deal with Windows specific stuff (clang-cl (FlagsToInclude/FlagsToExclude), lld COFF case-insensitive options).
• (b) parseOneArg deals with GNU getopt_long style command line.

The two use cases are different and no utility wants to have both features (no overlap). (a) added sufficient complexity to ParseOneArg. I don't want to make it more complex. The duplicated stuff is small - just 50 lines of code.

[grimar]

llvm/include/llvm/Option/OptTable.h

211

How about renaming this to parseOneArgGrouped and making it private?

[MaskRay]

parseOneArg (public) -> parseOneArgGrouped (private)

[jhenderson]

llvm/include/llvm/Option/Option.h

224–226

It might be nice to keep the Index parameter in the same place as it was before (i.e. move CurArg after it). I don't feel strongly about this though if you prefer your current approach.

llvm/lib/Option/OptTable.cpp

348

"Search for an option which matches Str."

[grimar]

This looks fine to me probably. 3 last nits.

llvm/include/llvm/Option/OptTable.h

94

Doesn't seem you need doxygen comments for a private method?

llvm/lib/Option/OptTable.cpp

380

++Index

llvm/lib/Option/Option.cpp

111

why unsigned and not size_t?

[MaskRay]

Address comments

llvm/include/llvm/Option/Option.h

224–226

I want to keep out parameters in the end.

llvm/lib/Option/OptTable.cpp

380

++Index changes semantics.

[grimar]

llvm/include/llvm/Option/OptTable.h

94

Nor addressed?

[jhenderson]

Hesitant LGTM. I don't really follow much of the command line behaviour, so best wait for others to approve too.

[grimar]

In D83639#2157629, @jhenderson wrote:

Hesitant LGTM. I don't really follow much of the command line behaviour, so best wait for others to approve too.

I do not feel myself confident in accepting changes for the lib/Option code too. So +1 hesistant LGTM with the unaddressed nit resolved somehow.
(I've never saw "///" comments used for protected/private methods I think, I am not sure it worth documenting them so hard,
since they are not a part of a public API. So I'd expect the comment for the parseOneArgGrouped to be rewritten/reduced probably).

[MaskRay]

Drop parseOneArgGrouped's comment in OptTable.h
(Moved to OptTable.cpp)

llvm/include/llvm/Option/ArgList.h

InputArgList &operator=(InputArgList &&RHS) {
     return *this;
   }
 
   ~InputArgList() { releaseMemory(); }
 
   const char *getArgString(unsigned Index) const override {
     return ArgStrings[Index];
   }
 
+  void replaceArgString(unsigned Index, const Twine &S) {
+    ArgStrings[Index] = MakeArgString(S);
+  }
+
   unsigned getNumInputArgStrings() const override {
     return NumInputArgStrings;
   }
 
   /// @name Arg Synthesis
   /// @{
 
 public:

llvm/include/llvm/Option/OptTable.h

struct Info {
     const char *AliasArgs;
     const char *Values;
   };
 
 private:
   /// The option information table.
   std::vector<Info> OptionInfos;
   bool IgnoreCase;
+  bool GroupedShortOptions = false;
 
   unsigned TheInputOptionID = 0;
   unsigned TheUnknownOptionID = 0;
 
   /// The index of the first option which can be parsed (i.e., is not a
   /// special option like 'input' or 'unknown', and is not an option group).
   unsigned FirstSearchableIndex = 0;
 
   /// The union of all option prefixes. If an argument does not begin with
   /// one of these, it is an input.
   StringSet<> PrefixesUnion;
   std::string PrefixChars;
 
 private:
   const Info &getInfo(OptSpecifier Opt) const {
     unsigned id = Opt.getID();
     assert(id > 0 && id - 1 < getNumOptions() && "Invalid Option ID.");
     return OptionInfos[id - 1];
   }
 
+  Arg *parseOneArgGrouped(InputArgList &Args, unsigned &Index) const;
+
 protected:
   OptTable(ArrayRef<Info> OptionInfos, bool IgnoreCase = false);
 
 public:
   ~OptTable();
 
   /// Return the total number of option classes.
   unsigned getNumOptions() const { return OptionInfos.size(); }
 
   /// Get the given Opt's Option instance, lazily creating it
   /// if necessary.
   ///
   /// \return The option, or null for the INVALID option id.
   const Option getOption(OptSpecifier Opt) const;
 
   /// Lookup the name of the given option.
   const char *getOptionName(OptSpecifier id) const {
     return getInfo(id).Name;
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;
   }
 
+  /// 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.
   ///
   /// \param [in] Option - Key flag like "-stdlib=" when "-stdlib=l"
   /// was passed to clang.
   ///
   /// \param [in] Arg - Value which we want to autocomplete like "l"
   /// when "-stdlib=l" was passed to clang.
public:
   ///
   /// \return The parsed argument, or 0 if the argument is missing values
   /// (in which case Index still points at the conceptual next argument string
   /// to parse).
   Arg *ParseOneArg(const ArgList &Args, unsigned &Index,
                    unsigned FlagsToInclude = 0,
                    unsigned FlagsToExclude = 0) const;
 
   /// Parse an list of arguments into an InputArgList.
   ///
   /// The resulting InputArgList will reference the strings in [\p ArgBegin,
   /// \p ArgEnd), and their lifetime should extend past that of the returned
   /// InputArgList.
   ///
   /// The only error that can occur in this routine is if an argument is
   /// missing values; in this case \p MissingArgCount will be non-zero.
   ///
   /// \param MissingArgIndex - On error, the index of the option which could
   /// not be parsed.
   /// \param MissingArgCount - On error, the number of missing options.
   /// \param FlagsToInclude - Only parse options with any of these flags.
   /// Zero is the default which includes all flags.
   /// \param FlagsToExclude - Don't parse options with this flag.  Zero
   /// is the default and means exclude nothing.
   /// \return An InputArgList; on error this will contain all the options
   /// which could be parsed.
   InputArgList ParseArgs(ArrayRef<const char *> Args, unsigned &MissingArgIndex,
                          unsigned &MissingArgCount, unsigned FlagsToInclude = 0,

llvm/include/llvm/Option/Option.h

public:
   /// matches - Predicate for whether this option is part of the
   /// given option (which may be a group).
   ///
   /// Note that matches against options which are an alias should never be
   /// done -- aliases do not participate in matching and so such a query will
   /// always be false.
   bool matches(OptSpecifier ID) const;
 
   /// accept - Potentially accept the current argument, returning a
   /// new Arg instance, or 0 if the option does not accept this
   /// argument (or the argument is missing values).
   ///
   /// If the option accepts the current argument, accept() sets
   /// Index to the position where argument parsing should resume
   /// (even if the argument is missing values).
   ///
-  /// \param ArgSize The number of bytes taken up by the matched Option prefix
-  ///                and name. This is used to determine where joined values
-  ///                start.
-  Arg *accept(const ArgList &Args, unsigned &Index, unsigned ArgSize) const;
+  /// \p CurArg The argument to be matched. It may be shorter than the
+  /// underlying storage to represent a Joined argument.
+  /// \p GroupedShortOption If true, we are handling the fallback case of
+  /// parsing a prefix of the current argument as a short option.
+  Arg *accept(const ArgList &Args, StringRef CurArg, bool GroupedShortOption,
+              unsigned &Index) const;
 
 private:
-  Arg *acceptInternal(const ArgList &Args, unsigned &Index,
-                      unsigned ArgSize) const;
+  Arg *acceptInternal(const ArgList &Args, StringRef CurArg,
+                      unsigned &Index) const;
 
 public:
   void print(raw_ostream &O) const;
   void dump() const;
 };
 
 } // end namespace opt
 
 } // end namespace llvm
 
 #endif // LLVM_OPTION_OPTION_H

llvm/lib/Option/OptTable.cpp

for (size_t I = FirstSearchableIndex, E = OptionInfos.size(); I < E; I++) {
     if (optionMatches(In, Option)) {
       In.Values = Values;
       return true;
     }
   }
   return false;
 }
 
+// Parse a single argument, return the new argument, and update Index. If
+// GroupedShortOptions is true, -a matches "-abc" and the argument in Args will
+// be updated to "-bc". This overload does not support
+// FlagsToInclude/FlagsToExclude or case insensitive options.
+Arg *OptTable::parseOneArgGrouped(InputArgList &Args, unsigned &Index) const {
+  // Anything that doesn't start with PrefixesUnion is an input, as is '-'
+  // itself.
+  const char *CStr = Args.getArgString(Index);
+  StringRef Str(CStr);
+  if (isInput(PrefixesUnion, Str))
+    return new Arg(getOption(TheInputOptionID), Str, Index++, CStr);
+
+  const Info *End = OptionInfos.data() + OptionInfos.size();
+  StringRef Name = Str.ltrim(PrefixChars);
+  const Info *Start = std::lower_bound(
+      OptionInfos.data() + FirstSearchableIndex, End, Name.data());
+  const Info *Fallback = nullptr;
+  unsigned Prev = Index;
+
+  // Search for the option which matches Str.
+  for (; Start != End; ++Start) {
+    unsigned ArgSize = matchOption(Start, Str, IgnoreCase);
+    if (!ArgSize)
+      continue;
+
+    Option Opt(Start, this);
+    if (Arg *A = Opt.accept(Args, StringRef(Args.getArgString(Index), ArgSize),
+                            false, Index))
+      return A;
+
+    // If Opt is a Flag of length 2 (e.g. "-a"), we know it is a prefix of
+    // the current argument (e.g. "-abc"). Match it as a fallback if no longer
+    // option (e.g. "-ab") exists.
+    if (ArgSize == 2 && Opt.getKind() == Option::FlagClass)
+      Fallback = Start;
+
+    // Otherwise, see if the argument is missing.
+    if (Prev != Index)
+      return nullptr;
+  }
+  if (Fallback) {
+    Option Opt(Fallback, this);
+    if (Arg *A = Opt.accept(Args, Str.substr(0, 2), true, Index)) {
+      if (Str.size() == 2)
+        ++Index;
+      else
+        Args.replaceArgString(Index, Twine('-') + Str.substr(2));
+      return A;
+    }
+  }
+
+  return new Arg(getOption(TheUnknownOptionID), Str, Index++, CStr);
+}
+
 Arg *OptTable::ParseOneArg(const ArgList &Args, unsigned &Index,
                            unsigned FlagsToInclude,
                            unsigned FlagsToExclude) const {
   unsigned Prev = Index;
   const char *Str = Args.getArgString(Index);
 
   // Anything that doesn't start with PrefixesUnion is an input, as is '-'
   // itself.
for (; Start != End; ++Start) {
     Option Opt(Start, this);
 
     if (FlagsToInclude && !Opt.hasFlag(FlagsToInclude))
       continue;
     if (Opt.hasFlag(FlagsToExclude))
       continue;
 
     // See if this option matches.
-    if (Arg *A = Opt.accept(Args, Index, ArgSize))
+    if (Arg *A = Opt.accept(Args, StringRef(Args.getArgString(Index), ArgSize),
+                            false, Index))
       return A;
 
     // Otherwise, see if this argument was missing values.
     if (Prev != Index)
       return nullptr;
   }
 
   // If we failed to find an option and this arg started with /, then it's
while (Index < End) {
     // Ignore empty arguments (other things may still take them as arguments).
     StringRef Str = Args.getArgString(Index);
     if (Str == "") {
       ++Index;
       continue;
     }
 
     unsigned Prev = Index;
-    Arg *A = ParseOneArg(Args, Index, FlagsToInclude, FlagsToExclude);
-    assert(Index > Prev && "Parser failed to consume argument.");
+    Arg *A = GroupedShortOptions
+                 ? parseOneArgGrouped(Args, Index)
+                 : ParseOneArg(Args, Index, FlagsToInclude, FlagsToExclude);
+    assert((Index > Prev || GroupedShortOptions) &&
+           "Parser failed to consume argument.");
 
     // Check for missing argument error.
     if (!A) {
       assert(Index >= End && "Unexpected parser error.");
       assert(Index - Prev - 1 && "No missing arguments!");
       MissingArgIndex = Prev;
       MissingArgCount = Index - Prev - 1;
       break;

llvm/lib/Option/Option.cpp

if (getID() == Opt.getID())
     return true;
 
   const Option Group = getGroup();
   if (Group.isValid())
     return Group.matches(Opt);
   return false;
 }
 
-Arg *Option::acceptInternal(const ArgList &Args, unsigned &Index,
-                            unsigned ArgSize) const {
-  StringRef Spelling = StringRef(Args.getArgString(Index), ArgSize);
+Arg *Option::acceptInternal(const ArgList &Args, StringRef Spelling,
+                            unsigned &Index) const {
+  size_t ArgSize = Spelling.size();
   switch (getKind()) {
   case FlagClass: {
     if (ArgSize != strlen(Args.getArgString(Index)))
       return nullptr;
     return new Arg(*this, Spelling, Index++);
   }
   case JoinedClass: {
     const char *Value = Args.getArgString(Index) + ArgSize;
case RemainingArgsJoinedClass: {
     return A;
   }
 
   default:
     llvm_unreachable("Invalid option kind!");
   }
 }
 
-Arg *Option::accept(const ArgList &Args,
-                    unsigned &Index,
-                    unsigned ArgSize) const {
-  std::unique_ptr<Arg> A(acceptInternal(Args, Index, ArgSize));
+Arg *Option::accept(const ArgList &Args, StringRef CurArg,
+                    bool GroupedShortOption, unsigned &Index) const {
+  std::unique_ptr<Arg> A(GroupedShortOption && getKind() == FlagClass
+                             ? new Arg(*this, CurArg, Index)
+                             : acceptInternal(Args, CurArg, Index));
   if (!A)
     return nullptr;
 
   const Option &UnaliasedOption = getUnaliasedOption();
   if (getID() == UnaliasedOption.getID())
     return A.release();
 
   // "A" is an alias for a different flag. For most clients it's more convenient

llvm/unittests/Option/OptionParsingTest.cpp

TEST(DISABLED_Option, FindNearestFIXME) {
 
   // FIXME: Options with joined values should not have those values considered
   // when calculating distance. The test below would fail if run, but it should
   // succeed.
   EXPECT_EQ(1U, T.findNearest("--erbghFoo", Nearest));
   EXPECT_EQ(Nearest, "--ermghFoo");
 
 }
+
+TEST(Option, ParseGroupedShortOptions) {
+  TestOptTable T;
+  T.setGroupedShortOptions(true);
+  unsigned MAI, MAC;
+
+  // Grouped short options can be followed by a long Flag (-Joo), or a non-Flag
+  // option (-C=1).
+  const char *Args1[] = {"-AIJ", "-AIJoo", "-AC=1"};
+  InputArgList AL = T.ParseArgs(Args1, MAI, MAC);
+  EXPECT_TRUE(AL.hasArg(OPT_A));
+  EXPECT_TRUE(AL.hasArg(OPT_H));
+  ASSERT_EQ((size_t)2, AL.getAllArgValues(OPT_B).size());
+  EXPECT_EQ("foo", AL.getAllArgValues(OPT_B)[0]);
+  EXPECT_EQ("bar", AL.getAllArgValues(OPT_B)[1]);
+  ASSERT_TRUE(AL.hasArg(OPT_C));
+  EXPECT_EQ("1", AL.getAllArgValues(OPT_C)[0]);
+
+  // Prefer a long option to a short option.
+  const char *Args2[] = {"-AB"};
+  InputArgList AL2 = T.ParseArgs(Args2, MAI, MAC);
+  EXPECT_TRUE(!AL2.hasArg(OPT_A));
+  EXPECT_TRUE(AL2.hasArg(OPT_AB));
+
+  // Short options followed by a long option. We probably should disallow this.
+  const char *Args3[] = {"-AIblorp"};
+  InputArgList AL3 = T.ParseArgs(Args3, MAI, MAC);
+  EXPECT_TRUE(AL3.hasArg(OPT_A));
+  EXPECT_TRUE(AL3.hasArg(OPT_Blorp));
+}
+
+TEST(Option, UnknownOptions) {
+  TestOptTable T;
+  unsigned MAI, MAC;
+  const char *Args[] = {"-u", "--long", "0"};
+  for (int I = 0; I < 2; ++I) {
+    T.setGroupedShortOptions(I != 0);
+    InputArgList AL = T.ParseArgs(Args, MAI, MAC);
+    const std::vector<std::string> Unknown = AL.getAllArgValues(OPT_UNKNOWN);
+    ASSERT_EQ((size_t)2, Unknown.size());
+    EXPECT_EQ("-u", Unknown[0]);
+    EXPECT_EQ("--long", Unknown[1]);
+  }
+}

llvm/unittests/Option/Opts.td

 include "llvm/Option/OptParser.td"
 
 def OptFlag1 : OptionFlag;
 def OptFlag2 : OptionFlag;
 def OptFlag3 : OptionFlag;
 
 def A : Flag<["-"], "A">, HelpText<"The A option">, Flags<[OptFlag1]>;
+def AB : Flag<["-"], "AB">;
 def B : Joined<["-"], "B">, HelpText<"The B option">, MetaVarName<"B">, Flags<[OptFlag2]>;
 def C : Separate<["-"], "C">, HelpText<"The C option">, MetaVarName<"C">, Flags<[OptFlag1]>;
 def SLASH_C : Separate<["/", "-"], "C">, HelpText<"The C option">, MetaVarName<"C">, Flags<[OptFlag3]>;
 def D : CommaJoined<["-"], "D">, HelpText<"The D option">, MetaVarName<"D">;
 def E : MultiArg<["-"], "E", 2>, Flags<[OptFlag1, OptFlag2]>;
 def F : JoinedOrSeparate<["-"], "F">, HelpText<"The F option">, MetaVarName<"F">;
 def G : JoinedAndSeparate<["-"], "G">, HelpText<"The G option">, MetaVarName<"G">;