dexonsmith added a comment.
I like this direction; the logic in the `.td` files seems much cleaner. The
`MARSHALLING` macro logic seems a bit harder to follow and there may be a bug,
but I'm not sure. See comments inline.
================
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);
\
----------------
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.
================
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));
\
}
----------------
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.
================
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));
\
}
----------------
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
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
