[PATCH] D91861: [clang][cli] Split DefaultAnyOf into a default value and ImpliedByAnyOf
Jan Svoboda via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Nov 23 03:08:54 PST 2020
jansvoboda11 added a comment.
Thanks for the feedback. I left some comments inline and will update the patch accordingly.
================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3762-3764
+ this->KEYPATH = MERGER(this->KEYPATH, DEFAULT_VALUE); \
+ if (IMPLIED_CHECK) \
+ this->KEYPATH = MERGER(this->KEYPATH, IMPLIED_VALUE); \
----------------
dexonsmith wrote:
> This flip to the logic is interesting. I see it now matches the non-flag case; I can't remember if there was a subtle reason it was different before though.
This makes more sense to me personally and I saw similar change in a later patch D84674. Let me know if you remember why it wasn't like that already.
================
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:
> 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)); \
}
```
================
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:
> 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.
================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:4038-4043
if (((FLAGS)&options::CC1Option) && \
- (ALWAYS_EMIT || EXTRACTOR(this->KEYPATH) != DEFAULT_VALUE)) { \
+ (ALWAYS_EMIT || \
+ (EXTRACTOR(this->KEYPATH) != DEFAULT_VALUE && !(IMPLIED_CHECK)))) { \
DENORMALIZER(Args, SPELLING, NEG_SPELLING, SA, TABLE_INDEX, \
EXTRACTOR(this->KEYPATH)); \
}
----------------
dexonsmith wrote:
> dexonsmith wrote:
> > I'm not entirely sure if the comment applies here, since a `bool` option is simpler, but it would be good to have tests to demonstrate correct behaviour for options with the following scenarios:
> > - option != default, it can be implied but the antecedents are false
> > - option == default, it can be implied but the antecedents are false
> > - option != default, it can be implied and the antecedents are true
> > - option == default, it can be implied and the antecedents are true
> (Maybe the tests already exist in tree; if so, please just point me at them)
The `bool` case works with the current logic, because there are only 2 options and typically, the implied value is negation of the default one.
I will change the logic to match what is discussed above to be consistent.
================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:4038-4043
if (((FLAGS)&options::CC1Option) && \
- (ALWAYS_EMIT || EXTRACTOR(this->KEYPATH) != DEFAULT_VALUE)) { \
+ (ALWAYS_EMIT || \
+ (EXTRACTOR(this->KEYPATH) != DEFAULT_VALUE && !(IMPLIED_CHECK)))) { \
DENORMALIZER(Args, SPELLING, NEG_SPELLING, SA, TABLE_INDEX, \
EXTRACTOR(this->KEYPATH)); \
}
----------------
jansvoboda11 wrote:
> dexonsmith wrote:
> > dexonsmith wrote:
> > > I'm not entirely sure if the comment applies here, since a `bool` option is simpler, but it would be good to have tests to demonstrate correct behaviour for options with the following scenarios:
> > > - option != default, it can be implied but the antecedents are false
> > > - option == default, it can be implied but the antecedents are false
> > > - option != default, it can be implied and the antecedents are true
> > > - option == default, it can be implied and the antecedents are true
> > (Maybe the tests already exist in tree; if so, please just point me at them)
> The `bool` case works with the current logic, because there are only 2 options and typically, the implied value is negation of the default one.
> I will change the logic to match what is discussed above to be consistent.
We don't test all cases currently, I'll fix that.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D91861/new/
https://reviews.llvm.org/D91861
More information about the llvm-commits
mailing list