[PATCH] D91861: [clang][cli] Split DefaultAnyOf into a default value and ImpliedByAnyOf

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

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