dexonsmith added inline comments.
================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:4027-4031
+ if (((FLAGS)&options::CC1Option) &&
\
+ (ALWAYS_EMIT ||
\
+ (EXTRACTOR(this->KEYPATH) != DEFAULT_VALUE && !(IMPLIED_CHECK)))) {
\
DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX, EXTRACTOR(this->KEYPATH));
\
}
----------------
jansvoboda11 wrote:
> jansvoboda11 wrote:
> > dexonsmith wrote:
> > > I'm not sure this logic is quite right. It looks to me like if an option
> > > can very be implied, it will never be seriazed to `-cc1`, even if its
> > > current value is not an implied one.
> > >
> > > Or have I understood the conditions under which `IMPLIED_CHECK` returns
> > > `true`?
> > >
> > > IIUC, then this logic seems closer:
> > > ```
> > > if (((FLAGS)&options::CC1Option) &&
> > > \
> > > (ALWAYS_EMIT ||
> > > \
> > > (EXTRACTOR(this->KEYPATH) !=
> > > \
> > > (IMPLIED_CHECK ? (DEFAULT_VALUE)
> > > \
> > > : (MERGER(DEFAULT_VALUE, IMPLIED_VALUE)))))) {
> > > \
> > > DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX,
> > > EXTRACTOR(this->KEYPATH)); \
> > > }
> > > ```
> > >
> > > It would be great to see tests in particular for a bitset (or similar)
> > > option where the merger does a union.
> > `IMPLIED_CHECK` is a logical disjunction of the implying option keypaths.
> > It evaluates to `true` whenever at least one of the implying keypaths
> > evaluates to `true`.
> >
> > I think I know what you're concerned about. Let me paraphrase it and check
> > if my understanding is correct:
> > Suppose option `a` has default value of `x`, and flag `b` can imply the
> > value of `a` to be `y`. If we have a command line `-b -a=z`, then `-a=z`
> > would not be generated with the current logic: `EXTRACTOR(this->KEYPATH) !=
> > DEFAULT_VALUE` evaluates to true, but `!(IMPLIED_CHECK)` to `false`.
> >
> > Your conditions gets close, but I think the ternary branches should be the
> > other way around.
> >
> > Here's a table exploring all cases:
> >
> > ```
> > IMPLIED_CHECK | EXTRACTOR(this->KEYPATH) == | SHOULD_EMIT
> > --------------+-----------------------------+-----------------------------------------
> > true | IMPLIED_VALUE | NO - emitting only the
> > implying option is enough
> > true | DEFAULT_VALUE | YES - value explicitly
> > specified (and it's DEFAULT_VALUE just by chance)
> > true | ??? | YES - value explicitly
> > specified
> > false | IMPLIED_VALUE | YES - value explicitly
> > specified (and it's IMPLIED_VALUE just by chance)
> > false | DEFAULT_VALUE | NO - default value handles
> > this automatically
> > false | ??? | YES - value explicitly
> > specified
> > ```
> >
> > I think this logic is what we're looking for:
> >
> > ```
> > if (((FLAGS)&options::CC1Option) &&
> > \
> > (ALWAYS_EMIT ||
> > \
> > (EXTRACTOR(this->KEYPATH) !=
> > \
> > ((IMPLIED_CHECK) ? (IMPLIED_VALUE) : (DEFAULT_VALUE))))) {
> > \
> > DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX,
> > EXTRACTOR(this->KEYPATH)); \
> > }
> > ```
> It would be great to be able to test this logic even when no Clang options
> exercise it properly yet. Any ideas on that front?
> Including a different `Options.inc` in `CompilerInvocation.cpp` for tests and
> re-linking the whole library seems wasteful.
That'd be great, but I don't see a simple way. Can you find a flag that would
exercise it properly? If so, I suggest converting just that one flag as part of
this patch, and then you can test it.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D91861/new/
https://reviews.llvm.org/D91861
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
