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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
