[PATCH] D97069: [clang] BoolMOption helper in Options.td

[Konstantin Zhuravlyov via Phabricator via llvm-commits]

kzhuravl added inline comments.


================
Comment at: clang/include/clang/Driver/Options.td:431
+// specified by the OptGroup argument, otherwise Group<m_Group>.
+multiclass BoolMOption<string flag_base,
+                       string HelpStringEnabled, string HelpStringDisabled,
----------------
jansvoboda11 wrote:
> jansvoboda11 wrote:
> > rampitec wrote:
> > > jansvoboda11 wrote:
> > > > rampitec wrote:
> > > > > jansvoboda11 wrote:
> > > > > > Can you please rename this to `BoolMFlag` and model it after `BoolFFlag`?
> > > > > > 
> > > > > > The `Bool?Option` multiclasses were created for fine-grained control of cc1 flag marshalling, which your implementation doesn't do/need. https://clang.llvm.org/docs/InternalsManual.html#option-marshalling-infrastructure
> > > > > It does not seem that BoolFFlag exists?
> > > > Ah, sorry, I meant `OptInFFlag` and `OptOutFFlag`.
> > > > 
> > > > You probably won't need the `KeyPathAndMacro` and `enablers` parameters, as you're not targeting cc1.
> > > It end up with the same multiclass, just different name? After OptInFFlag is cleaned from marshalling, kpm, enablers and CC1 stuff it looks more or less the same as this one. Plus to get the same behavior as now we need separate help strings and optional group.
> > My concern here is that naming this class `BoolMOption` will cause confusion. We already have `BoolFOption` and `BoolGOption` that are used for different purpose (automatic option marshalling in cc1).
> > 
> > Note: I'm in the process of converting `Opt{In,Out}FFlag` instances that perform marshalling via the `kpm` and `enablers` parameters to `BoolFOption` instances. My end-goal is to have clear distinction between marshalling multiclasses (ending with `Option`) and simple multiclasses that don't do marshalling (ending with `Flag`).
> > 
> > Since your multiclass doesn't perform marshalling, I'd strongly prefer the name to end with `Flag`. And since it's neither opt-in or opt-out (because it doesn't target cc1), how about naming it `SimpleMFlag`, `PlainMFlag` or something similar?
> > 
> > I also think keeping the API similar to existing multiclasses would be nice from user perspective. How about using the existing the `Opt{In,Out}FFlag` signature (stripped of `kpm` and `enablers`?
> > 
> > ```
> > string name, string pos_prefix, string neg_prefix="", string help="", list<OptionFlag> flags=[]
> > ```
> > 
> > It supports separate help strings and you can append `OptionGroup OptGroup = m_Group` at the end to fit your use-case.
> > 
> > I understand it will end up looking a lot like `Opt{In,Out}FFlag`, but the absence of `CC1Option` is pretty important, so I think it's fine.
> See D97370.
Would it make sense to rename OptInFFlag to OptInFlag, and take common prefix as a template arg, something like:


```
multiclass OptInFlag<**string prefix**, string name, string pos_prefix, string neg_prefix="",
                      string help="", list<OptionFlag> flags=[]> {
```

Then we will have existing "optin flags" as

defm cuda_short_ptr : OptInFlag<"f", "cuda-short-ptr",

And new "optin flags" as

defm tgsplit : OptInFlag<"m", "tgsplit",

?


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

https://reviews.llvm.org/D97069