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

[Yevgeny Rouban via Phabricator via llvm-commits]

yrouban added inline comments.


================
Comment at: llvm/include/llvm/Support/CommandLine.h:1409
   // type.
-  opt_storage() : Value(DataType()), Default(DataType()) {}
+  opt_storage() : Value(DataType()), Default() {}
 
----------------
hintonda wrote:
> yrouban wrote:
> > do you want me to add a comment?
> >   // Do not initialize Default. Keep it None.
> > 
> This [[ https://github.com/llvm/llvm-project/commit/fcb37243f9906b1f2ea4c1aeff0a3438288df41f | change ]] was explicitly made in 2013, so we need to be careful not to break anything that depends on it.
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.



================
Comment at: llvm/unittests/Support/CommandLineTest.cpp:1793
+  StackOption<bool> OptA("a", cl::desc("option a"), cl::init(false));
+  StackOption<bool> OptB(cl::init(false),
       "b", cl::desc("option b -- This option turns on option a"),
----------------
hintonda wrote:
> Are these additional `cl::init` calls needed? 
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.



================
Comment at: llvm/unittests/Support/CommandLineTest.cpp:1843
   EXPECT_TRUE(cl::ParseCommandLineOptions(2, args5));
   EXPECT_FALSE(OptA);
   EXPECT_FALSE(OptB);
----------------
Without explicit cl::init(false) for the option "a" it is left not reset at this here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114645/new/

https://reviews.llvm.org/D114645