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