[CommandLine] Do not crash if an option has both ValueRequired and Grouping.

[ikudrin]

Summary

If an option, which requires a value, has a cl::Grouping formatting flag,
it works well as far as it is used at the end of a group (or not in a group).
However, if the option appears accidentally in the middle of a group,
the program just crashes. I believe it is better to show some useful
error message in that case.

[jhenderson]

According to the documentation (https://llvm.org/docs/CommandLine.html#controlling-other-formatting-options) this is illegal even for the last value:

Note that cl::Grouping options cannot have values.

Does it just work? If so, great, but please update the docs.

Regardless, we certainly shouldn't be asserting if we can get into that state, so this is a good change.

Your description mentioned ValueRequired. What happens for ValueOptional? I want to make sure that it doesn't treat the other grouped letters after it as the option's value.

unittests/Support/CommandLineTest.cpp

1146 ↗

(On Diff #187753)

The first option isn't really in the middle, after all, so this comment should be adjusted to say "elswhere in the group" or "not at the end of a group" or similar.

[ikudrin]

• Updated the documentation (feel free to correct my wording).
• Amended a comment in the test.

[ikudrin]

In D58499#1405561, @jhenderson wrote:

What happens for ValueOptional? I want to make sure that it doesn't treat the other grouped letters after it as the option's value.

As far as I can see, in case of grouped options, the value might be assigned only if it is a prefixed option. Did you mean a specific case? Could you provide it so that I understand you better?

Anyway, if anything needs fixing with ValueOptional, it should be a separate patch, right?

[jhenderson]

LGTM.

In D58499#1407211, @ikudrin wrote:

In D58499#1405561, @jhenderson wrote:

What happens for ValueOptional? I want to make sure that it doesn't treat the other grouped letters after it as the option's value.

As far as I can see, in case of grouped options, the value might be assigned only if it is a prefixed option. Did you mean a specific case? Could you provide it so that I understand you better?

Anyway, if anything needs fixing with ValueOptional, it should be a separate patch, right?

Yes, you're right. If there's any issue with ValueOptional, it should be a separate patch. I don't have a specific case in mind. I just want to make sure that the behaviour of something like "-abc" where -a, -b and -c are all options, and -b is ValueOptional makes sense (I'm not sure whether it should be equivalent to "-a -b=c" or "-a -b -c" though). Although the code change is purely to do with ValueRequired, the documentation change is not specific to ValueRequired. Perhaps you could do a follow-up patch with at least a test for this behaviour, if it just works?

CC'ing @rupprecht, who may be interested in this change too.

[ikudrin]

In D58499#1408521, @jhenderson wrote:

LGTM.

Thanks!

I just want to make sure that the behaviour of something like "-abc" where -a, -b and -c are all options, and -b is ValueOptional makes sense (I'm not sure whether it should be equivalent to "-a -b=c" or "-a -b -c" though).

If I understand it right, a ValueOptional option can accept its value only if it is explicitly used in the -opt=value form. Moreover, as boolean options are of a ValueOptional kind implicitly, your sample is something very basic for checking how Grouping works at all. "-abc" should definitely be equivalent to "-a -b -c". It is a bit strange that there are no tests for that. Maybe the test suite is worth to be extended.

Although the code change is purely to do with ValueRequired, the documentation change is not specific to ValueRequired.

You are right. We either need better wording in the documentation or improve the code so that the "=val" form is accepted with a group. I would prefer changing the documentation because no one needed that new behavior yet.

Perhaps you could do a follow-up patch with at least a test for this behaviour, if it just works?

Let's create a separate patch with additional tests for the existing behavior.

llvm/trunk/docs/CommandLine.rst

 
 .. _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
-  cannot have values.
+  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.
 
 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

llvm/trunk/lib/Support/CommandLine.cpp

HandlePrefixedOrGroupedOption(StringRef &Arg, StringRef &Value,
   // have values.
   assert(isGrouping(PGOpt) && "Broken getOptionPred!");
 
   do {
     // Move current arg name out of Arg into OneArgName.
     StringRef OneArgName = Arg.substr(0, Length);
     Arg = Arg.substr(Length);
 
-    // Because ValueRequired is an invalid flag for grouped arguments,
-    // we don't need to pass argc/argv in.
-    assert(PGOpt->getValueExpectedFlag() != cl::ValueRequired &&
-           "Option can not be cl::Grouping AND cl::ValueRequired!");
+    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);
 
     // Get the next grouping option.
     PGOpt = getOptionPred(Arg, Length, isGrouping, OptionsMap);
   } while (PGOpt && Length != Arg.size());
 

llvm/trunk/unittests/Support/CommandLineTest.cpp

TEST(CommandLineTest, PrefixOptions) {
   EXPECT_TRUE(MacroDefs.empty());
   const char *args6[] = {"prog", "-DHAVE_FOO"};
   EXPECT_TRUE(
       cl::ParseCommandLineOptions(2, args6, StringRef(), &llvm::nulls()));
   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<std::string> OptV("v", cl::Grouping,
+                                cl::desc("Grouping option with a value"));
+
+  // 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());
+  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()));
+}
+
 }  // anonymous namespace