Add support for prefix-only CLI options

[thopre]

Summary

Add support for options that always prefix their value, giving an error
if the value is in the next argument or if the option is given a value
assignment (ie. opt=val). This is the desired behavior for the -D option
of FileCheck for instance.

Copyright:

• Linaro (changes in version 2 of revision D55940)
• GraphCore (changes in later versions and introduced when creating D56549)

[thopre]

Fix comment in LookupOption for AlwaysPrefix early bail out

[thopre]

Clarify what happens in LookupOption when failing to match an option.

[jdenny]

I commented when this was part of D55940. However, I'm not as familiar with this area, especially the unit testing, so someone else should review further.

[probinson]

Nice work on the prefix tests. One comment nit and LGTM.

llvm/lib/Support/CommandLine.cpp

547

... or the option only supports ...

llvm/include/llvm/Support/CommandLine.h

 //
 // Prefix - This option allows arguments that are otherwise unrecognized to be
 // matched by options that are a prefix of the actual value.  This is useful for
 // 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?
-  Grouping = 0x03          // Can this option group with other options?
+  AlwaysPrefix = 0x03,     // Can this option only directly prefix its value?
+  Grouping = 0x04          // Can this option group with other options?
 };
 
 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?
 };
 
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 : 2; // enum FormattingFlags
+  unsigned Formatting : 3; // enum FormattingFlags
   unsigned Misc : 3;
   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

llvm/lib/Support/CommandLine.cpp

if (EqualPos == StringRef::npos) {
     // Look up the option.
     auto I = Sub.OptionsMap.find(Arg);
     if (I == Sub.OptionsMap.end())
       return nullptr;
 
     return I != Sub.OptionsMap.end() ? I->second : nullptr;
   }
 
-  // If the argument before the = is a valid option name, we match.  If not,
-  // return Arg unmolested.
+  // If the argument before the = is a valid option name and the option allows
+  // non-prefix form (ie is not AlwaysPrefix), we match.  If not, signal match
+  // failure by returning nullptr.
   auto I = Sub.OptionsMap.find(Arg.substr(0, EqualPos));
   if (I == Sub.OptionsMap.end())
     return nullptr;
 
+  auto O = I->second;
+  if (O->getFormattingFlag() == cl::AlwaysPrefix)
+    return nullptr;
+
   Value = Arg.substr(EqualPos + 1);
   Arg = Arg.substr(0, EqualPos);
   return I->second;
 }
 
 SubCommand *CommandLineParser::LookupSubCommand(StringRef Name) {
   if (Name.empty())
     return &*TopLevelSubCommand;
static inline bool ProvideOption(Option *Handler, StringRef ArgName,
                                  const char *const *argv, int &i) {
   // Is this a multi-argument option?
   unsigned NumAdditionalVals = Handler->getNumAdditionalVals();
 
   // Enforce value requirements
   switch (Handler->getValueExpectedFlag()) {
   case ValueRequired:
     if (!Value.data()) { // No value specified?
-      if (i + 1 >= argc)
+      // If no other argument or option only support prefix form, we cannot
+      // look at next argument.
+      if (i + 1 >= argc || Handler->getFormattingFlag() == cl::AlwaysPrefix)
         return Handler->error("requires a value!");
       // Steal the next argument, like for '-o filename'
       assert(argv && "null check");
       Value = StringRef(argv[++i]);
     }
     break;
   case ValueDisallowed:
     if (NumAdditionalVals > 0)
static bool ProvidePositionalOption(Option *Handler, StringRef Arg, int i) {
   return ProvideOption(Handler, Handler->ArgStr, Arg, 0, nullptr, Dummy);
 }
 
 // Option predicates...
 static inline bool isGrouping(const Option *O) {
   return O->getFormattingFlag() == cl::Grouping;
 }
 static inline bool isPrefixedOrGrouping(const Option *O) {
-  return isGrouping(O) || O->getFormattingFlag() == cl::Prefix;
+  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
 // checked by progressively stripping characters off of the name, checking to
 // see if there options that satisfy the predicate.  If we find one, return it,
 // otherwise return null.
 //
HandlePrefixedOrGroupedOption(StringRef &Arg, StringRef &Value,
   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) {
+  if (PGOpt->getFormattingFlag() == cl::Prefix ||
+      PGOpt->getFormattingFlag() == cl::AlwaysPrefix) {
     Value = Arg.substr(Length);
     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.

llvm/unittests/Support/CommandLineTest.cpp

TEST(CommandLineTest, GetCommandLineArguments) {
 
   EXPECT_TRUE(llvm::sys::path::filename(argv[0])
               .equals_lower("supporttests.exe"))
       << "Filename of test executable is "
       << llvm::sys::path::filename(argv[0]);
 }
 #endif
 
-}  // anonymous namespace
+TEST(CommandLineTest, PrefixOptions) {
+  cl::ResetCommandLineParser();
+
+  StackOption<std::string, cl::list<std::string>> IncludeDirs(
+      "I", cl::Prefix, cl::desc("Declare an include directory"));
+
+  // Test non-prefixed variant works with cl::Prefix options.
+  EXPECT_TRUE(IncludeDirs.empty());
+  const char *args[] = {"prog", "-I=/usr/include"};
+  EXPECT_TRUE(
+      cl::ParseCommandLineOptions(2, args, StringRef(), &llvm::nulls()));
+  EXPECT_TRUE(IncludeDirs.size() == 1);
+  EXPECT_TRUE(IncludeDirs.front().compare("/usr/include") == 0);
+
+  IncludeDirs.erase(IncludeDirs.begin());
+  cl::ResetAllOptionOccurrences();
+
+  // Test non-prefixed variant works with cl::Prefix options when value is
+  // passed in following argument.
+  EXPECT_TRUE(IncludeDirs.empty());
+  const char *args2[] = {"prog", "-I", "/usr/include"};
+  EXPECT_TRUE(
+      cl::ParseCommandLineOptions(3, args2, StringRef(), &llvm::nulls()));
+  EXPECT_TRUE(IncludeDirs.size() == 1);
+  EXPECT_TRUE(IncludeDirs.front().compare("/usr/include") == 0);
+
+  IncludeDirs.erase(IncludeDirs.begin());
+  cl::ResetAllOptionOccurrences();
+
+  // Test prefixed variant works with cl::Prefix options.
+  EXPECT_TRUE(IncludeDirs.empty());
+  const char *args3[] = {"prog", "-I/usr/include"};
+  EXPECT_TRUE(
+      cl::ParseCommandLineOptions(2, args3, StringRef(), &llvm::nulls()));
+  EXPECT_TRUE(IncludeDirs.size() == 1);
+  EXPECT_TRUE(IncludeDirs.front().compare("/usr/include") == 0);
+
+  StackOption<std::string, cl::list<std::string>> MacroDefs(
+      "D", cl::AlwaysPrefix, cl::desc("Define a macro"),
+      cl::value_desc("MACRO[=VALUE]"));
+
+  cl::ResetAllOptionOccurrences();
+
+  // Test non-prefixed variant does not work with cl::AlwaysPrefix options:
+  // equal sign is part of the value.
+  EXPECT_TRUE(MacroDefs.empty());
+  const char *args4[] = {"prog", "-D=HAVE_FOO"};
+  EXPECT_TRUE(
+      cl::ParseCommandLineOptions(2, args4, StringRef(), &llvm::nulls()));
+  EXPECT_TRUE(MacroDefs.size() == 1);
+  EXPECT_TRUE(MacroDefs.front().compare("=HAVE_FOO") == 0);
+
+  MacroDefs.erase(MacroDefs.begin());
+  cl::ResetAllOptionOccurrences();
+
+  // Test non-prefixed variant does not allow value to be passed in following
+  // argument with cl::AlwaysPrefix options.
+  EXPECT_TRUE(MacroDefs.empty());
+  const char *args5[] = {"prog", "-D", "HAVE_FOO"};
+  EXPECT_FALSE(
+      cl::ParseCommandLineOptions(3, args5, StringRef(), &llvm::nulls()));
+  EXPECT_TRUE(MacroDefs.empty());
+
+  cl::ResetAllOptionOccurrences();
+
+  // Test prefixed variant works with cl::AlwaysPrefix options.
+  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);
+}
+
+} // anonymous namespace