[CommandLine] Allow grouping options which can have values.

This patch allows all forms of values for options to be used at the end
of a group. With the fix, it is possible to follow the way GNU binutils
tools handle grouping options better. For example, the -j option can be
used with objdump in any of the following ways:

$ objdump -d -j .text a.o
$ objdump -d -j.text a.o
$ objdump -dj .text a.o
$ objdump -dj.text a.o

Differential Revision: https://reviews.llvm.org/D58711

llvm-svn: 355185

Author: igorkudrin

llvm/docs/CommandLine.rst

@@ -1168,12 +1168,18 @@ As usual, you can only specify one of these arguments at most.
 .. _grouping:
 .. _cl::Grouping:
 
-* The **cl::Grouping** modifier is used to implement Unix-style tools (like
-  ``ls``) that have lots of single letter arguments, but only require a single
-  dash.  For example, the '``ls -labF``' command actually enables four different
-  options, all of which are single letters.  Note that **cl::Grouping** options
-  can have values only if they are used separately or at the end of the groups.
-  It is a runtime error if such an option is used elsewhere in the group.
+Controlling options grouping
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+The **cl::Grouping** modifier can be combined with any formatting types except
+for `cl::Positional`_.  It is used to implement Unix-style tools (like ``ls``)
+that have lots of single letter arguments, but only require a single dash.
+For example, the '``ls -labF``' command actually enables four different options,
+all of which are single letters.
+
+Note that **cl::Grouping** options can have values only if they are used
+separately or at the end of the groups.  For `cl::ValueRequired`_, it is
+a runtime error if such an option is used elsewhere in the group.
 
 The CommandLine library does not restrict how you use the **cl::Prefix** or
 **cl::Grouping** modifiers, but it is possible to specify ambiguous argument
@@ -1188,19 +1194,24 @@ basically looks like this:
 
   parse(string OrigInput) {
 
-  1. string input = OrigInput;
-  2. if (isOption(input)) return getOption(input).parse();  // Normal option
-  3. while (!isOption(input) && !input.empty()) input.pop_back();  // Remove the last letter
-  4. if (input.empty()) return error();  // No matching option
-  5. if (getOption(input).isPrefix())
-       return getOption(input).parse(input);
-  6. while (!input.empty()) {  // Must be grouping options
-       getOption(input).parse();
-       OrigInput.erase(OrigInput.begin(), OrigInput.begin()+input.length());
-       input = OrigInput;
-       while (!isOption(input) && !input.empty()) input.pop_back();
+  1. string Input = OrigInput;
+  2. if (isOption(Input)) return getOption(Input).parse();  // Normal option
+  3. while (!Input.empty() && !isOption(Input)) Input.pop_back();  // Remove the last letter
+  4. while (!Input.empty()) {
+       string MaybeValue = OrigInput.substr(Input.length())
+       if (getOption(Input).isPrefix())
+         return getOption(Input).parse(MaybeValue)
+       if (!MaybeValue.empty() && MaybeValue[0] == '=')
+         return getOption(Input).parse(MaybeValue.substr(1))
+       if (!getOption(Input).isGrouping())
+         return error()
+       getOption(Input).parse()
+       Input = OrigInput = MaybeValue
+       while (!Input.empty() && !isOption(Input)) Input.pop_back();
+       if (!Input.empty() && !getOption(Input).isGrouping())
+         return error()
      }
-  7. if (!OrigInput.empty()) error();
+  5. if (!OrigInput.empty()) error();
 
   }
 
@@ -1240,8 +1251,6 @@ specify boolean properties that modify the option.
   with ``cl::CommaSeparated``, this modifier only makes sense with a `cl::list`_
   option.
 
-So far, these are the only three miscellaneous option modifiers.
-
 .. _response files:
 
 Response files

llvm/include/llvm/Support/CommandLine.h

@@ -158,23 +158,24 @@ enum OptionHidden {   // Control whether -help shows this option
 // AlwaysPrefix - Only allow the behavior enabled by the Prefix flag and reject
 // the Option=Value form.
 //
-// Grouping - With this option enabled, multiple letter options are allowed to
-// bunch together with only a single hyphen for the whole group.  This allows
-// emulation of the behavior that ls uses for example: ls -la === ls -l -a
-//
 
 enum FormattingFlags {
   NormalFormatting = 0x00, // Nothing special
   Positional = 0x01,       // Is a positional argument, no '-' required
   Prefix = 0x02,           // Can this option directly prefix its value?
-  AlwaysPrefix = 0x03,     // Can this option only directly prefix its value?
-  Grouping = 0x04          // Can this option group with other options?
+  AlwaysPrefix = 0x03      // Can this option only directly prefix its value?
 };
 
 enum MiscFlags {             // Miscellaneous flags to adjust argument
   CommaSeparated = 0x01,     // Should this cl::list split between commas?
   PositionalEatsArgs = 0x02, // Should this positional cl::list eat -args?
-  Sink = 0x04                // Should this cl::list eat all unknown options?
+  Sink = 0x04,               // Should this cl::list eat all unknown options?
+
+  // Grouping - Can this option group with other options?
+  // If this is enabled, multiple letter options are allowed to bunch together
+  // with only a single hyphen for the whole group.  This allows emulation
+  // of the behavior that ls uses for example: ls -la === ls -l -a
+  Grouping = 0x08
 };
 
 //===----------------------------------------------------------------------===//
@@ -268,8 +269,8 @@ class Option {
   // detail representing the non-value
   unsigned Value : 2;
   unsigned HiddenFlag : 2; // enum OptionHidden
-  unsigned Formatting : 3; // enum FormattingFlags
-  unsigned Misc : 3;
+  unsigned Formatting : 2; // enum FormattingFlags
+  unsigned Misc : 4;
   unsigned Position = 0;       // Position of last occurrence of the option
   unsigned AdditionalVals = 0; // Greater than 0 for multi-valued option.
 

llvm/lib/Support/CommandLine.cpp

@@ -600,7 +600,7 @@ static bool ProvidePositionalOption(Option *Handler, StringRef Arg, int i) {
 
 // Option predicates...
 static inline bool isGrouping(const Option *O) {
-  return O->getFormattingFlag() == cl::Grouping;
+  return O->getMiscFlags() & cl::Grouping;
 }
 static inline bool isPrefixedOrGrouping(const Option *O) {
   return isGrouping(O) || O->getFormattingFlag() == cl::Prefix ||
@@ -651,26 +651,29 @@ HandlePrefixedOrGroupedOption(StringRef &Arg, StringRef &Value,
   if (!PGOpt)
     return nullptr;
 
-  // If the option is a prefixed option, then the value is simply the
-  // rest of the name...  so fall through to later processing, by
-  // setting up the argument name flags and value fields.
-  if (PGOpt->getFormattingFlag() == cl::Prefix ||
-      PGOpt->getFormattingFlag() == cl::AlwaysPrefix) {
-    Value = Arg.substr(Length);
+  do {
+    StringRef MaybeValue =
+        (Length < Arg.size()) ? Arg.substr(Length) : StringRef();
     Arg = Arg.substr(0, Length);
     assert(OptionsMap.count(Arg) && OptionsMap.find(Arg)->second == PGOpt);
-    return PGOpt;
-  }
 
-  // This must be a grouped option... handle them now.  Grouping options can't
-  // have values.
-  assert(isGrouping(PGOpt) && "Broken getOptionPred!");
+    // cl::Prefix options do not preserve '=' when used separately.
+    // The behavior for them with grouped options should be the same.
+    if (MaybeValue.empty() || PGOpt->getFormattingFlag() == cl::AlwaysPrefix ||
+        (PGOpt->getFormattingFlag() == cl::Prefix && MaybeValue[0] != '=')) {
+      Value = MaybeValue;
+      return PGOpt;
+    }
+
+    if (MaybeValue[0] == '=') {
+      Value = MaybeValue.substr(1);
+      return PGOpt;
+    }
 
-  do {
-    // Move current arg name out of Arg into OneArgName.
-    StringRef OneArgName = Arg.substr(0, Length);
-    Arg = Arg.substr(Length);
+    // This must be a grouped option.
+    assert(isGrouping(PGOpt) && "Broken getOptionPred!");
 
+    // Grouping options inside a group can't have values.
     if (PGOpt->getValueExpectedFlag() == cl::ValueRequired) {
       ErrorParsing |= PGOpt->error("may not occur within a group!");
       return nullptr;
@@ -679,15 +682,15 @@ HandlePrefixedOrGroupedOption(StringRef &Arg, StringRef &Value,
     // Because the value for the option is not required, we don't need to pass
     // argc/argv in.
     int Dummy = 0;
-    ErrorParsing |=
-        ProvideOption(PGOpt, OneArgName, StringRef(), 0, nullptr, Dummy);
+    ErrorParsing |= ProvideOption(PGOpt, Arg, StringRef(), 0, nullptr, Dummy);
 
     // Get the next grouping option.
+    Arg = MaybeValue;
     PGOpt = getOptionPred(Arg, Length, isGrouping, OptionsMap);
-  } while (PGOpt && Length != Arg.size());
+  } while (PGOpt);
 
-  // Return the last option with Arg cut down to just the last one.
-  return PGOpt;
+  // We could not find a grouping option in the remainder of Arg.
+  return nullptr;
 }
 
 static bool RequiresValue(const Option *O) {

llvm/tools/llvm-readobj/llvm-readobj.cpp

@@ -688,7 +688,7 @@ static void registerReadelfAliases() {
     StringRef ArgName = OptEntry.getKey();
     cl::Option *Option = OptEntry.getValue();
     if (ArgName.size() == 1)
-      Option->setFormattingFlag(cl::Grouping);
+      apply(Option, cl::Grouping);
   }
 }
 

llvm/unittests/Support/CommandLineTest.cpp

@@ -1132,8 +1132,13 @@ TEST(CommandLineTest, GroupingWithValue) {
   cl::ResetCommandLineParser();
 
   StackOption<bool> OptF("f", cl::Grouping, cl::desc("Some flag"));
+  StackOption<bool> OptB("b", cl::Grouping, cl::desc("Another flag"));
+  StackOption<bool> OptD("d", cl::Grouping, cl::ValueDisallowed,
+                         cl::desc("ValueDisallowed option"));
   StackOption<std::string> OptV("v", cl::Grouping,
-                                cl::desc("Grouping option with a value"));
+                                cl::desc("ValueRequired option"));
+  StackOption<std::string> OptO("o", cl::Grouping, cl::ValueOptional,
+                                cl::desc("ValueOptional option"));
 
   // Should be possible to use an option which requires a value
   // at the end of a group.
@@ -1142,12 +1147,178 @@ TEST(CommandLineTest, GroupingWithValue) {
       cl::ParseCommandLineOptions(3, args1, StringRef(), &llvm::nulls()));
   EXPECT_TRUE(OptF);
   EXPECT_STREQ("val1", OptV.c_str());
+  OptV.clear();
   cl::ResetAllOptionOccurrences();
 
   // Should not crash if it is accidentally used elsewhere in the group.
   const char *args2[] = {"prog", "-vf", "val2"};
   EXPECT_FALSE(
       cl::ParseCommandLineOptions(3, args2, StringRef(), &llvm::nulls()));
+  OptV.clear();
+  cl::ResetAllOptionOccurrences();
+
+  // Should allow the "opt=value" form at the end of the group
+  const char *args3[] = {"prog", "-fv=val3"};
+  EXPECT_TRUE(
+      cl::ParseCommandLineOptions(2, args3, StringRef(), &llvm::nulls()));
+  EXPECT_TRUE(OptF);
+  EXPECT_STREQ("val3", OptV.c_str());
+  OptV.clear();
+  cl::ResetAllOptionOccurrences();
+
+  // Should allow assigning a value for a ValueOptional option
+  // at the end of the group
+  const char *args4[] = {"prog", "-fo=val4"};
+  EXPECT_TRUE(
+      cl::ParseCommandLineOptions(2, args4, StringRef(), &llvm::nulls()));
+  EXPECT_TRUE(OptF);
+  EXPECT_STREQ("val4", OptO.c_str());
+  OptO.clear();
+  cl::ResetAllOptionOccurrences();
+
+  // Should assign an empty value if a ValueOptional option is used elsewhere
+  // in the group.
+  const char *args5[] = {"prog", "-fob"};
+  EXPECT_TRUE(
+      cl::ParseCommandLineOptions(2, args5, StringRef(), &llvm::nulls()));
+  EXPECT_TRUE(OptF);
+  EXPECT_EQ(1, OptO.getNumOccurrences());
+  EXPECT_EQ(1, OptB.getNumOccurrences());
+  EXPECT_TRUE(OptO.empty());
+  cl::ResetAllOptionOccurrences();
+
+  // Should not allow an assignment for a ValueDisallowed option.
+  const char *args6[] = {"prog", "-fd=false"};
+  EXPECT_FALSE(
+      cl::ParseCommandLineOptions(2, args6, StringRef(), &llvm::nulls()));
+}
+
+TEST(CommandLineTest, GroupingAndPrefix) {
+  cl::ResetCommandLineParser();
+
+  StackOption<bool> OptF("f", cl::Grouping, cl::desc("Some flag"));
+  StackOption<bool> OptB("b", cl::Grouping, cl::desc("Another flag"));
+  StackOption<std::string> OptP("p", cl::Prefix, cl::Grouping,
+                                cl::desc("Prefix and Grouping"));
+  StackOption<std::string> OptA("a", cl::AlwaysPrefix, cl::Grouping,
+                                cl::desc("AlwaysPrefix and Grouping"));
+
+  // Should be possible to use a cl::Prefix option without grouping.
+  const char *args1[] = {"prog", "-pval1"};
+  EXPECT_TRUE(
+      cl::ParseCommandLineOptions(2, args1, StringRef(), &llvm::nulls()));
+  EXPECT_STREQ("val1", OptP.c_str());
+  OptP.clear();
+  cl::ResetAllOptionOccurrences();
+
+  // Should be possible to pass a value in a separate argument.
+  const char *args2[] = {"prog", "-p", "val2"};
+  EXPECT_TRUE(
+      cl::ParseCommandLineOptions(3, args2, StringRef(), &llvm::nulls()));
+  EXPECT_STREQ("val2", OptP.c_str());
+  OptP.clear();
+  cl::ResetAllOptionOccurrences();
+
+  // The "-opt=value" form should work, too.
+  const char *args3[] = {"prog", "-p=val3"};
+  EXPECT_TRUE(
+      cl::ParseCommandLineOptions(2, args3, StringRef(), &llvm::nulls()));
+  EXPECT_STREQ("val3", OptP.c_str());
+  OptP.clear();
+  cl::ResetAllOptionOccurrences();
+
+  // All three previous cases should work the same way if an option with both
+  // cl::Prefix and cl::Grouping modifiers is used at the end of a group.
+  const char *args4[] = {"prog", "-fpval4"};
+  EXPECT_TRUE(
+      cl::ParseCommandLineOptions(2, args4, StringRef(), &llvm::nulls()));
+  EXPECT_TRUE(OptF);
+  EXPECT_STREQ("val4", OptP.c_str());
+  OptP.clear();
+  cl::ResetAllOptionOccurrences();
+
+  const char *args5[] = {"prog", "-fp", "val5"};
+  EXPECT_TRUE(
+      cl::ParseCommandLineOptions(3, args5, StringRef(), &llvm::nulls()));
+  EXPECT_TRUE(OptF);
+  EXPECT_STREQ("val5", OptP.c_str());
+  OptP.clear();
+  cl::ResetAllOptionOccurrences();
+
+  const char *args6[] = {"prog", "-fp=val6"};
+  EXPECT_TRUE(
+      cl::ParseCommandLineOptions(2, args6, StringRef(), &llvm::nulls()));
+  EXPECT_TRUE(OptF);
+  EXPECT_STREQ("val6", OptP.c_str());
+  OptP.clear();
+  cl::ResetAllOptionOccurrences();
+
+  // Should assign a value even if the part after a cl::Prefix option is equal
+  // to the name of another option.
+  const char *args7[] = {"prog", "-fpb"};
+  EXPECT_TRUE(
+      cl::ParseCommandLineOptions(2, args7, StringRef(), &llvm::nulls()));
+  EXPECT_TRUE(OptF);
+  EXPECT_STREQ("b", OptP.c_str());
+  EXPECT_FALSE(OptB);
+  OptP.clear();
+  cl::ResetAllOptionOccurrences();
+
+  // Should be possible to use a cl::AlwaysPrefix option without grouping.
+  const char *args8[] = {"prog", "-aval8"};
+  EXPECT_TRUE(
+      cl::ParseCommandLineOptions(2, args8, StringRef(), &llvm::nulls()));
+  EXPECT_STREQ("val8", OptA.c_str());
+  OptA.clear();
+  cl::ResetAllOptionOccurrences();
+
+  // Should not be possible to pass a value in a separate argument.
+  const char *args9[] = {"prog", "-a", "val9"};
+  EXPECT_FALSE(
+      cl::ParseCommandLineOptions(3, args9, StringRef(), &llvm::nulls()));
+  cl::ResetAllOptionOccurrences();
+
+  // With the "-opt=value" form, the "=" symbol should be preserved.
+  const char *args10[] = {"prog", "-a=val10"};
+  EXPECT_TRUE(
+      cl::ParseCommandLineOptions(2, args10, StringRef(), &llvm::nulls()));
+  EXPECT_STREQ("=val10", OptA.c_str());
+  OptA.clear();
+  cl::ResetAllOptionOccurrences();
+
+  // All three previous cases should work the same way if an option with both
+  // cl::AlwaysPrefix and cl::Grouping modifiers is used at the end of a group.
+  const char *args11[] = {"prog", "-faval11"};
+  EXPECT_TRUE(
+      cl::ParseCommandLineOptions(2, args11, StringRef(), &llvm::nulls()));
+  EXPECT_TRUE(OptF);
+  EXPECT_STREQ("val11", OptA.c_str());
+  OptA.clear();
+  cl::ResetAllOptionOccurrences();
+
+  const char *args12[] = {"prog", "-fa", "val12"};
+  EXPECT_FALSE(
+      cl::ParseCommandLineOptions(3, args12, StringRef(), &llvm::nulls()));
+  cl::ResetAllOptionOccurrences();
+
+  const char *args13[] = {"prog", "-fa=val13"};
+  EXPECT_TRUE(
+      cl::ParseCommandLineOptions(2, args13, StringRef(), &llvm::nulls()));
+  EXPECT_TRUE(OptF);
+  EXPECT_STREQ("=val13", OptA.c_str());
+  OptA.clear();
+  cl::ResetAllOptionOccurrences();
+
+  // Should assign a value even if the part after a cl::AlwaysPrefix option
+  // is equal to the name of another option.
+  const char *args14[] = {"prog", "-fab"};
+  EXPECT_TRUE(
+      cl::ParseCommandLineOptions(2, args14, StringRef(), &llvm::nulls()));
+  EXPECT_TRUE(OptF);
+  EXPECT_STREQ("b", OptA.c_str());
+  EXPECT_FALSE(OptB);
+  OptA.clear();
+  cl::ResetAllOptionOccurrences();
 }
 
 }  // anonymous namespace