[CommandLine] Fix cl::ConsumeAfter support with more than one positional argument

[Yi-Hong.Lyu]

Summary

Currently, cl::ConsumeAfter only works for the case that has exactly one
positional argument. Without the fix, it skip fulfilling first positional
argument and put that additional positional argument in interpreter arguments.

[Yi-Hong.Lyu]

Address clang-tidy's warning and rename local variable for readability.

[Yi-Hong.Lyu]

Add a test ConsumeAfterOnePositional

[Yi-Hong.Lyu]

Ping ...

[Yi-Hong.Lyu]

ping

[rnk]

ConsumeAfter is used in three places: lli, KillTheDoctor, and modularize, so I wouldn't be surprised if there was an off-by-one error here.

Starting the index here at 1 dates back to the original CommandLine 2.0 check in (rG5df56c47fc5c), so I'm hesitant to say this is correct, but your unit tests and the lit suite for lli give me confidence that it does. Looks good to me, I guess.

llvm/lib/Support/CommandLine.cpp

for (size_t i = 0, e = PositionalOpts.size(); i != e; ++i) {
           llvm_unreachable("Internal error, unexpected NumOccurrences flag in "
                            "positional argument processing!");
         }
       }
     }
   } else {
     assert(ConsumeAfterOpt && NumPositionalRequired <= PositionalVals.size());
     unsigned ValNo = 0;
-    for (size_t j = 1, e = PositionalOpts.size(); j != e; ++j)
-      if (RequiresValue(PositionalOpts[j])) {
-        ErrorParsing |= ProvidePositionalOption(PositionalOpts[j],
+    for (size_t J = 0, E = PositionalOpts.size(); J != E; ++J)
+      if (RequiresValue(PositionalOpts[J])) {
+        ErrorParsing |= ProvidePositionalOption(PositionalOpts[J],
                                                 PositionalVals[ValNo].first,
                                                 PositionalVals[ValNo].second);
         ValNo++;
       }
 
     // Handle the case where there is just one positional option, and it's
     // optional.  In this case, we want to give JUST THE FIRST option to the
     // positional option and keep the rest for the consume after.  The above

llvm/unittests/Support/CommandLineTest.cpp

 
 enum Enum { Val1, Val2 };
 static cl::bits<Enum> ExampleBits(
     cl::desc("An example cl::bits to ensure it compiles"),
     cl::values(
       clEnumValN(Val1, "bits-val1", "The Val1 value"),
       clEnumValN(Val1, "bits-val2", "The Val2 value")));
 
+TEST(CommandLineTest, ConsumeAfterOnePositional) {
+  cl::ResetCommandLineParser();
+
+  // input [args]
+  StackOption<std::string, cl::opt<std::string>> Input(cl::Positional,
+                                                       cl::Required);
+  StackOption<std::string, cl::list<std::string>> ExtraArgs(cl::ConsumeAfter);
+
+  const char *Args[] = {"prog", "input", "arg1", "arg2"};
+
+  std::string Errs;
+  raw_string_ostream OS(Errs);
+  EXPECT_TRUE(cl::ParseCommandLineOptions(4, Args, StringRef(), &OS));
+  OS.flush();
+  EXPECT_EQ("input", Input);
+  EXPECT_TRUE(ExtraArgs.size() == 2);
+  EXPECT_TRUE(ExtraArgs[0] == "arg1");
+  EXPECT_TRUE(ExtraArgs[1] == "arg2");
+  EXPECT_TRUE(Errs.empty());
+}
+
+TEST(CommandLineTest, ConsumeAfterTwoPositionals) {
+  cl::ResetCommandLineParser();
+
+  // input1 input2 [args]
+  StackOption<std::string, cl::opt<std::string>> Input1(cl::Positional,
+                                                        cl::Required);
+  StackOption<std::string, cl::opt<std::string>> Input2(cl::Positional,
+                                                        cl::Required);
+  StackOption<std::string, cl::list<std::string>> ExtraArgs(cl::ConsumeAfter);
+
+  const char *Args[] = {"prog", "input1", "input2", "arg1", "arg2"};
+
+  std::string Errs;
+  raw_string_ostream OS(Errs);
+  EXPECT_TRUE(cl::ParseCommandLineOptions(5, Args, StringRef(), &OS));
+  OS.flush();
+  EXPECT_EQ("input1", Input1);
+  EXPECT_EQ("input2", Input2);
+  EXPECT_TRUE(ExtraArgs.size() == 2);
+  EXPECT_TRUE(ExtraArgs[0] == "arg1");
+  EXPECT_TRUE(ExtraArgs[1] == "arg2");
+  EXPECT_TRUE(Errs.empty());
+}
+
 } // anonymous namespace