[CommandLine] Allow grouping options which can have values.

[ikudrin]

Summary

This patch is a follow-up for D58499. It allows the remaining forms of providing values
for options to be used at the end of a group. This includes both prefix variants
of options and the "opt=val" form.

With this fix, it is possible to better follow the way GNU binutils tools handle grouping
options. For example, the -j option in objdump can be used 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

[jhenderson]

docs/CommandLine.rst

1201 ↗

(On Diff #188515)

The indentation here looks off compared to the old style?

lib/Support/CommandLine.cpp

682–683 ↗

(On Diff #188515)

Why has this changed?

692 ↗

(On Diff #188515)

remaining -> remainder

unittests/Support/CommandLineTest.cpp

1176 ↗

(On Diff #188515)

Maybe better to be explicit here instead of using "such", i.e. "if a ValueOptional option..."

1225 ↗

(On Diff #188515)

The all -> All

The wrapping here looks a bit weird. I think "if a" should be on this line, not the next.

1270–1271 ↗

(On Diff #188515)

Same as above comment re. "The all" and "if a".

1289 ↗

(On Diff #188515)

What about a test case for "-fab" where "a", "f" and "b" are all options, and "a" is a Prefix/AlwaysPrefix option? I think that would clearly show that "b" would be the value of "a".

[ikudrin]

• Fixed grammar and formatting issues.
• Added test cases to check prefix options when their values are textually equal to another option.

[jhenderson]

LGTM.

[ikudrin]

By the way, we have some inconsistency in how cl::Prefix and cl::AlwaysPrefix options work in case of -opt=val form. The former takes only val, while the latter keeps =. Moreover, prefix grouping options in GNU's binutils, like -j in objdump, work partially as ours cl::AlwaysPrefix, because they preserve =, and partially as cl::Prefix, because they allow passing the value in a separate argument. Thus, we still cannot fully reproduce that behavior.

Do you think it is worth adding a new modifier, say, cl::PreserveEqSign, to improve compatibility?

unittests/Support/CommandLineTest.cpp

1225 ↗

(On Diff #188515)

I wanted not to split cl::Prefix + cl::Grouping, as well as not to separate an article from them.
So, I opted to rephrase the comment. How does it look now?

[jhenderson]

In D58711#1413416, @ikudrin wrote:

Do you think it is worth adding a new modifier, say, cl::PreserveEqSign, to improve compatibility?

I'm not really sure. I don't know enough about the background of the CommandLine library and its aims, so I'm reluctant to suggest adding more modifiers for what might be corner behaviours that aren't used. Perhaps worth putting up a small RFC on llvm-dev?

unittests/Support/CommandLineTest.cpp

1225 ↗

(On Diff #188515)

Yes, it's fine.

[rupprecht]

Great, thanks!

unittests/Support/CommandLineTest.cpp

1145 ↗

(On Diff #188688)

Can these test cases also assert that OptF is true?

llvm/trunk/docs/CommandLine.rst

* The **cl::Prefix** modifier specifies that this option prefixes its value.
   options are normal string (or list) options, that have the **cl::Prefix**
   modifier added to allow the CommandLine library to recognize them.  Note that
   **cl::Prefix** options must not have the **cl::ValueDisallowed** modifier
   specified.
 
 .. _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
 settings.  Thus, it is possible to have multiple letter options that are prefix
 or grouping options, and they will still work as designed.
 
 To do this, the CommandLine library uses a greedy algorithm to parse the input
 option into (potentially multiple) prefix and grouping options.  The strategy
 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();
 
   }
 
 Miscellaneous option modifiers
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 The miscellaneous option modifiers are the only flags where you can specify more
 than one flag from the set: they are not mutually exclusive.  These flags

 .. _cl::Sink:
 
 * The **cl::Sink** modifier is used to handle unknown options. If there is at
   least one option with ``cl::Sink`` modifier specified, the parser passes
   unrecognized option strings to it as values instead of signaling an error. As
   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
 ^^^^^^^^^^^^^^
 
 Some systems, such as certain variants of Microsoft Windows and some older
 Unices have a relatively low limit on command-line length. It is therefore
 customary to use the so-called 'response files' to circumvent this

llvm/trunk/include/llvm/Support/CommandLine.h

 // cases like a linker, where options are typically of the form '-lfoo' or
 // '-L../../include' where -l or -L are the actual flags.  When prefix is
 // enabled, and used, the value for the flag comes from the suffix of the
 // argument.
 //
 // 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
 };
 
 //===----------------------------------------------------------------------===//
 // Option Category class
 //
 class OptionCategory {
 private:
   StringRef const Name;
class Option {
   int NumOccurrences = 0; // The number of times specified
   // Occurrences, HiddenFlag, and Formatting are all enum types but to avoid
   // problems with signed enums in bitfields.
   unsigned Occurrences : 3; // enum NumOccurrencesFlag
   // not using the enum type for 'Value' because zero is an implementation
   // 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.
 
 public:
   StringRef ArgStr;   // The argument string itself (ex: "help", "o")
   StringRef HelpStr;  // The descriptive text message for -help
   StringRef ValueStr; // String describing what the value of this option is
   OptionCategory *Category; // The Category this option belongs to

llvm/trunk/lib/Support/CommandLine.cpp

 
 static bool ProvidePositionalOption(Option *Handler, StringRef Arg, int i) {
   int Dummy = i;
   return ProvideOption(Handler, Handler->ArgStr, Arg, 0, nullptr, Dummy);
 }
 
 // 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 ||
          O->getFormattingFlag() == cl::AlwaysPrefix;
 }
 
 // getOptionPred - Check to see if there are any options that satisfy the
 // specified predicate with names that are the prefixes in Name.  This is
if (Arg.size() == 1)
     return nullptr;
 
   // Do the lookup!
   size_t Length = 0;
   Option *PGOpt = getOptionPred(Arg, Length, isPrefixedOrGrouping, OptionsMap);
   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);
+
+    // 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;
     }
 
-  // This must be a grouped option... handle them now.  Grouping options can't
-  // have values.
-  assert(isGrouping(PGOpt) && "Broken getOptionPred!");
+    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;
     }
 
     // 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) {
   return O->getNumOccurrencesFlag() == cl::Required ||
          O->getNumOccurrencesFlag() == cl::OneOrMore;
 }
 
 static bool EatsUnboundedNumberOfValues(const Option *O) {

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

static cl::alias SymbolsShort("s", cl::desc("Alias for --symbols"),
                                 cl::aliasopt(opts::Symbols), cl::NotHidden,
                                 cl::Grouping);
 
   // Allow all single letter flags to be grouped together.
   for (auto &OptEntry : cl::getRegisteredOptions()) {
     StringRef ArgName = OptEntry.getKey();
     cl::Option *Option = OptEntry.getValue();
     if (ArgName.size() == 1)
-      Option->setFormattingFlag(cl::Grouping);
+      apply(Option, cl::Grouping);
   }
 }
 
 int main(int argc, const char *argv[]) {
   InitLLVM X(argc, argv);
 
   // Register the target printer for --version.
   cl::AddExtraVersionPrinter(TargetRegistry::printRegisteredTargetsForVersion);

llvm/trunk/unittests/Support/CommandLineTest.cpp

TEST(CommandLineTest, PrefixOptions) {
   EXPECT_TRUE(MacroDefs.size() == 1);
   EXPECT_TRUE(MacroDefs.front().compare("HAVE_FOO") == 0);
 }
 
 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.
   const char *args1[] = {"prog", "-fv", "val1"};
   EXPECT_TRUE(
       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