[PATCH] D114645: [CommandLine] Keep option default value unset if no cl::init() is used
Yevgeny Rouban via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Dec 7 22:07:46 PST 2021
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
More information about the llvm-commits
mailing list