Reset all options in cl::ResetCommandLineParser()

[csigg]

Summary

Reset cl::Positional, cl::Sink and cl::ConsumeAfter options as well in cl::ResetCommandLineParser().

[rriddle]

This looks right to me, but might be good to get another LGTM.

[csigg]

Rebase.

[csigg]

Sam, does this change look reasonable to you?

[sammccall]

Sorry for the delay. I'm not really familiar with the code, but this seems right (not sure if it's bulletproof, but surely not worse).

llvm/lib/Support/CommandLine.cpp

1332

As I understand these will typically be in OptionsMap too, unless ArgStr happens to be empty.
So we're possibly resetting the same options multiple times, and that's OK because it's cheap and idempotent.

This all seems fine but I'd leave a comment at the top like "Reset all options at least once, it's okay if some get reset multiple times".

(An alternative is to track which ones have been reset, or add a member containing this, but it doesn't seem worth it)

llvm/lib/Support/CommandLine.cpp

for (int I = 1; I < argc; ++I)
     NewArgv.push_back(argv[I]);
   int NewArgc = static_cast<int>(NewArgv.size());
 
   // Parse all options.
   return GlobalParser->ParseCommandLineOptions(NewArgc, &NewArgv[0], Overview,
                                                Errs, LongOptionsUseDoubleDash);
 }
 
+/// Reset all options at least once, so that we can parse different options.
 void CommandLineParser::ResetAllOptionOccurrences() {
-  // So that we can parse different command lines multiple times in succession
-  // we reset all option values to look like they have never been seen before.
+  // Reset all option values to look like they have never been seen before.
+  // Options might be reset twice (they can be reference in both OptionsMap
+  // and one of the other members), but that does not harm.
   for (auto *SC : RegisteredSubCommands) {
     for (auto &O : SC->OptionsMap)
       O.second->reset();
+    for (Option *O : SC->PositionalOpts)
+      O->reset();
+    for (Option *O : SC->SinkOpts)
+      O->reset();
+    if (SC->ConsumeAfterOpt)
+      SC->ConsumeAfterOpt->reset();
   }
 }
 
 bool CommandLineParser::ParseCommandLineOptions(int argc,
                                                 const char *const *argv,
                                                 StringRef Overview,
                                                 raw_ostream *Errs,
                                                 bool LongOptionsUseDoubleDash) {

llvm/unittests/Support/CommandLineTest.cpp

TEST(CommandLineTest, ConsumeAfterTwoPositionals) {
   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());
 }
 
+TEST(CommandLineTest, ResetAllOptionOccurrences) {
+  cl::ResetCommandLineParser();
+
+  // -option [sink] input [args]
+  StackOption<bool> Option("option");
+  StackOption<std::string, cl::list<std::string>> Sink(cl::Sink);
+  StackOption<std::string> Input(cl::Positional);
+  StackOption<std::string, cl::list<std::string>> ExtraArgs(cl::ConsumeAfter);
+
+  const char *Args[] = {"prog", "-option", "-unknown", "input", "-arg"};
+
+  std::string Errs;
+  raw_string_ostream OS(Errs);
+  EXPECT_TRUE(cl::ParseCommandLineOptions(5, Args, StringRef(), &OS));
+  EXPECT_TRUE(OS.str().empty());
+
+  EXPECT_TRUE(Option);
+  EXPECT_EQ(1, (int)Sink.size());
+  EXPECT_EQ("-unknown", Sink[0]);
+  EXPECT_EQ("input", Input);
+  EXPECT_EQ(1, (int)ExtraArgs.size());
+  EXPECT_EQ("-arg", ExtraArgs[0]);
+
+  cl::ResetAllOptionOccurrences();
+  EXPECT_FALSE(Option);
+  EXPECT_EQ(0, (int)Sink.size());
+  EXPECT_EQ(0, Input.getNumOccurrences());
+  EXPECT_EQ(0, (int)ExtraArgs.size());
+}
+
 } // anonymous namespace