[PATCH] D105881: [flang][driver] Switch to `BoolFOption` for boolean options

Tue Jul 13 05:57:21 PDT 2021

awarzynski added inline comments.

================
Comment at: clang/include/clang/Driver/Options.td:246

+class EmptyKPM<string base> : KeyPathAndMacro<"", "", ""> {}
 class DiagnosticOpts<string base>
----------------
jansvoboda11 wrote:
> awarzynski wrote:
> > @jansvoboda11 , is this the right approach here? I'd like use `BoolFOption` in Flang, but we don't need `KeyPathAndMacro` just yet. We may do in the future.
> There was an `EmptyKPM` class present in the code before, but only as an implementation detail of the marshalling system. People started using it with `BoolFOption` to conveniently declare pairs Clang driver flags. I'm not a fan since the whole `BooFOption` is designed around the keypath and its marshalling. Putting a "null" keypath in `BoolFOption` and then talking about its defaults and the effect of each flag seems highly unintuitive to me and it's essentially dead code. I'd prefer a different approach in this patch unless you have plans to switch to the marshalling infrastructure and remove `EmptyKPM` in the short term.
> 
> Would something similar to `OptInFFlag` and `OptOutFFlag` work for you? You could generalize it (remove `Flags<[CC1Option]>`), make use of it for Flang's options directly and forward the current `Opt{In,Out}FFlag` to the generalization while specifying the `CC1Option`. WDYT?
Thanks for the quick feedback! Initially when I saw `CC1Option` in `CC1Option`/`OptOutFFlag`, I immediately decided to explore alternatives. But I agree with everything that you said and am happy to generalize these multiclasses (instead of adding `EmptyKPM`). I don't see why it wouldn't work for what we currently need in Flang. I should be able to update this patch tomorrow.

Btw, what's the key benefit of the marshaling infra? Is that for being able to restore the original compiler invocation from instances of `CompilerInvocation`? We are still busy with other things, but definitely don't want Flang to miss out here.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105881