[CommandLine] Reset option to its default if its Default field is undefined

[yrouban]

Summary

This patch makes opt::setDefaultImpl() set the option value to the option type's default value if the Default field is not set. This results in option value reset by Option::reset() or ResetAllOptionOccurrences() even if the cl::init() is not specified.

Example:

StackOption<std::string> Str("str"); // No cl::init().
Str = "some value";
cl::ResetAllOptionOccurrences();
EXPECT_EQ("", Str); // The Str is reset.

[yrouban]

added context lines

[yrouban]

ping, please review.

[greened]

What is the motivation for this change? I can argue for maintaining the current behavior., If I pass a cl::init to an option and call ResetAllOptionOccurrences I would expect that cl::init value to be used to reset the option. There are compiler flows that rely on that behavior.

[beanz]

In D115433#3231665, @greened wrote:

What is the motivation for this change? I can argue for maintaining the current behavior., If I pass a cl::init to an option and call ResetAllOptionOccurrences I would expect that cl::init value to be used to reset the option. There are compiler flows that rely on that behavior.

IIUC, the point of this change is not to alter the behavior for options that have a cl::init, but rather to alter behavior for options that _don't_ have a default provided. Right now, those options don't get reset when ResetAllOptionOccurrences is called. With this patch, options _without_ cl::init should get reset to the default-constructed value of the option type, and options with cl::init get initialized to the default value.

[yrouban]

In D115433#3231806, @beanz wrote:

... With this patch, options _without_ cl::init should get reset to the default-constructed value of the option type, and options with cl::init get initialized to the default value.

+1. I have fixed the summary to make it clearer.

[greened]

IIUC, the point of this change is not to alter the behavior for options that have a cl::init, but rather to alter behavior for options that _don't_ have a default provided. Right now, those options don't get reset when ResetAllOptionOccurrences is called. With this patch, options _without_ cl::init should get reset to the default-constructed value of the option type, and options with cl::init get initialized to the default value.

Ah, ok, thank you!

[yrouban]

ping, please review.

[lattner]

Looks ok to me. To make code review smoother, I'd recommend making the motivation stronger in the commit message. Instead of starting by the "what", start by the goal, e.g.:

"Make ResetAllOptionOccurrences work better for xxx by doing YYY; this solves zz problem"

instead of "change low level behavior of something from xx to yy..."

[yrouban]

In D115433#3346412, @lattner wrote:

... To make code review smoother, I'd recommend making the motivation stronger in the commit message.

ok. Thank you Chris.

llvm/include/llvm/Support/CommandLine.h

class opt : public Option,
   }
 
   template <class T,
             class = std::enable_if_t<std::is_assignable<T &, T>::value>>
   void setDefaultImpl() {
     const OptionValue<DataType> &V = this->getDefault();
     if (V.hasValue())
       this->setValue(V.getValue());
+    else
+      this->setValue(T());
   }
 
   template <class T,
             class = std::enable_if_t<!std::is_assignable<T &, T>::value>>
   void setDefaultImpl(...) {}
 
   void setDefault() override { setDefaultImpl<DataType>(); }
 

llvm/unittests/Support/CommandLineTest.cpp

TEST(CommandLineTest, ConsumeAfterTwoPositionals) {
   EXPECT_EQ(ExtraArgs[0], "arg1");
   EXPECT_EQ(ExtraArgs[1], "arg2");
   EXPECT_TRUE(Errs.empty());
 }
 
 TEST(CommandLineTest, ResetAllOptionOccurrences) {
   cl::ResetCommandLineParser();
 
-  // -option -enableA -enableC [sink] input [args]
+  // -option -str -enableA -enableC [sink] input [args]
   StackOption<bool> Option("option");
+  StackOption<std::string> Str("str");
   enum Vals { ValA, ValB, ValC };
   StackOption<Vals, cl::bits<Vals>> Bits(
       cl::values(clEnumValN(ValA, "enableA", "Enable A"),
                  clEnumValN(ValB, "enableB", "Enable B"),
                  clEnumValN(ValC, "enableC", "Enable C")));
   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", "-enableA", "-enableC",
-                        "-unknown", "input",   "-arg"};
+  const char *Args[] = {"prog",     "-option",  "-str=STR", "-enableA",
+                        "-enableC", "-unknown", "input",    "-arg"};
 
   std::string Errs;
   raw_string_ostream OS(Errs);
-  EXPECT_TRUE(cl::ParseCommandLineOptions(7, Args, StringRef(), &OS));
+  EXPECT_TRUE(cl::ParseCommandLineOptions(8, Args, StringRef(), &OS));
   EXPECT_TRUE(OS.str().empty());
 
   EXPECT_TRUE(Option);
+  EXPECT_EQ("STR", Str);
   EXPECT_EQ((1u << ValA) | (1u << ValC), Bits.getBits());
   EXPECT_EQ(1u, Sink.size());
   EXPECT_EQ("-unknown", Sink[0]);
   EXPECT_EQ("input", Input);
   EXPECT_EQ(1u, ExtraArgs.size());
   EXPECT_EQ("-arg", ExtraArgs[0]);
 
   cl::ResetAllOptionOccurrences();
   EXPECT_FALSE(Option);
+  EXPECT_EQ("", Str);
   EXPECT_EQ(0u, Bits.getBits());
   EXPECT_EQ(0u, Sink.size());
   EXPECT_EQ(0, Input.getNumOccurrences());
   EXPECT_EQ(0u, ExtraArgs.size());
 }
 
 } // anonymous namespace