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

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

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