[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/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:
> yrouban wrote:
> > 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.
> > 
> 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.
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.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114645