[CommandLine] Keep option default value unset if no cl::init() is used

[yrouban]

Summary

Current declaration of cl::opt is incoherent between class and non-class specializations of the opt_storage template. There is an inconsistency in the initialization of the Default field: for inClass instances the default constructor is used - it sets the Optional Default field to None; though for non-inClass instances the Default field is set to the type's default value. For non-inClass instances it is impossible to know if the option is defined with cl::init() initializer or not:

cl::opt<int> i1("option-i1");
cl::opt<int> i2("option-i2", cl::init(0));
cl::opt<std::string> s1("option-s1");
cl::opt<std::string> s2("option-s2", cl::init(""));

assert(s1.Default.hasValue() != s2.Default.hasValue()); // Ok
assert(i1.Default.hasValue() != i2.Default.hasValue()); // Fails

This patch changes constructor of the non-class specializations to keep the Default field unset (that is None) rather than initialize it with DataType().

[yrouban]

Any thought? ping

[hintonda]

Thanks for looking into this. I'll take a look this afternoon.

Btw, your summary outlines the issue, but doesn't seem to indicate the fix -- could you add that, and perhaps comments where appropriate?

[yrouban]

llvm/include/llvm/Support/CommandLine.h

1369

do you want me to add a comment?

// Do not initialize Default. Keep it None.

[hintonda]

llvm/include/llvm/Support/CommandLine.h

1369

This change was explicitly made in 2013, so we need to be careful not to break anything that depends on it.

llvm/unittests/Support/CommandLineTest.cpp

1825

Are these additional cl::init calls needed?

[yrouban]

llvm/include/llvm/Support/CommandLine.h

1369

Thanks for pointing this out. That change looks to have a rationale limited to one particular option, which has been changed since then and got explicit cl::init value in LoopVectorize.cpp.

llvm/unittests/Support/CommandLineTest.cpp

1825

Yes, I found them needed as the test failed otherwise.

For example, the first cl::init(false) for the option "a" when reset by cl::ResetAllOptionOccurrences() ...

CommandLine.h:

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());
}

Without the explicit default value specified V.hasValue() is None and setDefaultValueImple<bool>() does not call setValue(). So the option is left not reset at line 1843.

1874

Without explicit cl::init(false) for the option "a" it is left not reset at this here.

[hintonda]

llvm/unittests/Support/CommandLineTest.cpp

1825

Ah, got it.

Would it be possible to tweak cl::ResetAllOptionOccurrences() instead? That way users don't have to modify their code.

I'm just worried about breaking code that's been around for a while.

[yrouban]

llvm/unittests/Support/CommandLineTest.cpp

1825

Logically, what is setDefaultImpl() supposed to do? reset the option to its default value (that is ValueType()) or reset it to the Default value (that is what was specified with cl::init())? I prepared a separate patch D115433 that resets to the Default if it is defined otherwise the option is reset to ValueType(). So these cl::init()s are not needed.

[yrouban]

Extracted a separate change D115433.

[yrouban]

ping, please review.

[yrouban]

ping, please review.

[lattner]

LGTM, thank you for improving this!

[yrouban]

In D114645#3344629, @lattner wrote:

LGTM, thank you for improving this!

Thank you very much. Consider reviewing the related
D115433 [CommandLine] Reset option to its default if its Default field is undefined

llvm/include/llvm/Support/CommandLine.h

 //
 template <class DataType> class opt_storage<DataType, false, false> {
 public:
   DataType Value;
   OptionValue<DataType> Default;
 
   // Make sure we initialize the value with the default constructor for the
   // type.
-  opt_storage() : Value(DataType()), Default(DataType()) {}
+  opt_storage() : Value(DataType()), Default() {}
 
   template <class T> void setValue(const T &V, bool initial = false) {
     Value = V;
     if (initial)
       Default = V;
   }
   DataType &getValue() { return Value; }
   DataType getValue() const { return Value; }

llvm/unittests/Support/CommandLineTest.cpp

TEST(CommandLineTest, OptionErrorMessageSuggestNoHidden) {
   cl::ResetAllOptionOccurrences();
 }
 
 TEST(CommandLineTest, Callback) {
   cl::ResetCommandLineParser();
 
   StackOption<bool> OptA("a", cl::desc("option a"));
   StackOption<bool> OptB(
       "b", cl::desc("option b -- This option turns on option a"),
       cl::callback([&](const bool &) { OptA = true; }));
   StackOption<bool> OptC(
       "c", cl::desc("option c -- This option turns on options a and b"),
       cl::callback([&](const bool &) { OptB = true; }));
   StackOption<std::string, cl::list<std::string>> List(
       "list",
       cl::desc("option list -- This option turns on options a, b, and c when "
                "'foo' is included in list"),
TEST(CommandLineTest, Callback) {
   EXPECT_TRUE(OptA);
   EXPECT_TRUE(OptB);
   EXPECT_TRUE(OptC);
   EXPECT_EQ(List.size(), 2u);
   cl::ResetAllOptionOccurrences();
 
   const char *args5[] = {"prog", "--list=bar"};
   EXPECT_TRUE(cl::ParseCommandLineOptions(2, args5));
   EXPECT_FALSE(OptA);
   EXPECT_FALSE(OptB);
   EXPECT_FALSE(OptC);
   EXPECT_EQ(List.size(), 1u);
 
   cl::ResetAllOptionOccurrences();
 }
 
 enum Enum { Val1, Val2 };
TEST(CommandLineTest, 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());
 }
 
+TEST(CommandLineTest, DefaultValue) {
+  cl::ResetCommandLineParser();
+
+  StackOption<bool> BoolOption("bool-option");
+  StackOption<std::string> StrOption("str-option");
+  StackOption<bool> BoolInitOption("bool-init-option", cl::init(true));
+  StackOption<std::string> StrInitOption("str-init-option",
+                                         cl::init("str-default-value"));
+
+  const char *Args[] = {"prog"}; // no options
+
+  std::string Errs;
+  raw_string_ostream OS(Errs);
+  EXPECT_TRUE(cl::ParseCommandLineOptions(1, Args, StringRef(), &OS));
+  EXPECT_TRUE(OS.str().empty());
+
+  EXPECT_TRUE(!BoolOption);
+  EXPECT_FALSE(BoolOption.Default.hasValue());
+  EXPECT_EQ(0, BoolOption.getNumOccurrences());
+
+  EXPECT_EQ("", StrOption);
+  EXPECT_FALSE(StrOption.Default.hasValue());
+  EXPECT_EQ(0, StrOption.getNumOccurrences());
+
+  EXPECT_TRUE(BoolInitOption);
+  EXPECT_TRUE(BoolInitOption.Default.hasValue());
+  EXPECT_EQ(0, BoolInitOption.getNumOccurrences());
+
+  EXPECT_EQ("str-default-value", StrInitOption);
+  EXPECT_TRUE(StrInitOption.Default.hasValue());
+  EXPECT_EQ(0, StrInitOption.getNumOccurrences());
+
+  const char *Args2[] = {"prog", "-bool-option", "-str-option=str-value",
+                         "-bool-init-option=0",
+                         "-str-init-option=str-init-value"};
+
+  EXPECT_TRUE(cl::ParseCommandLineOptions(5, Args2, StringRef(), &OS));
+  EXPECT_TRUE(OS.str().empty());
+
+  EXPECT_TRUE(BoolOption);
+  EXPECT_FALSE(BoolOption.Default.hasValue());
+  EXPECT_EQ(1, BoolOption.getNumOccurrences());
+
+  EXPECT_EQ("str-value", StrOption);
+  EXPECT_FALSE(StrOption.Default.hasValue());
+  EXPECT_EQ(1, StrOption.getNumOccurrences());
+
+  EXPECT_FALSE(BoolInitOption);
+  EXPECT_TRUE(BoolInitOption.Default.hasValue());
+  EXPECT_EQ(1, BoolInitOption.getNumOccurrences());
+
+  EXPECT_EQ("str-init-value", StrInitOption);
+  EXPECT_TRUE(StrInitOption.Default.hasValue());
+  EXPECT_EQ(1, StrInitOption.getNumOccurrences());
+}
+
 } // anonymous namespace