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

Duncan P. N. Exon Smith via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Nov 30 13:13:06 PST 2020


dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.

This LGTM. Thanks again for the cleanup, I think the new syntax is more intuitive.



================
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));   \
   }
----------------
dexonsmith wrote:
> 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.
On second read, perhaps the tests you added are good enough for now, as long as you're careful to improve the test coverage when you land the patches with options that have interesting merger/bitset logic. I'll leave it up to you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91861



More information about the cfe-commits mailing list