[PATCH] D83071: Add support for options with two flags for controlling the same field.

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

dexonsmith requested changes to this revision.
dexonsmith added inline comments.
This revision now requires changes to proceed.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:150
 
-static llvm::Optional<unsigned> normalizeSimpleEnum(OptSpecifier Opt,
-                                                    unsigned TableIndex,
----------------
I'm not sure if removing the `llvm::` namespace is intentional here, but if so please do it in a separate NFC patch to avoid adding noise in this one.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:161-163
+  if (!Args.hasArg(PosOpt, NegOpt))
+    return None;
+  return Args.hasFlag(PosOpt, NegOpt);
----------------
This should be:
```
if (Arg *A = Args.getLastArg(PosOpt, NegOpt))
  return A->getOption().matches(PosOpt);
return None;
```
Note that `hasArg` and `hasFlag` both resolve to `getLastArg`, so calling them one after another would be unfortunate.

... but I'm not even sure where `NegOpt` is coming from here, that looks like it hasn't been passed in. I think you need to change the signature to something like this:
```
static llvm::Optional<bool>
normalizeSimpleFlag(OptSpecifier Opt,
                    Optional<OptSpecifier> NegOpt,
                    unsigned TableIndex,
                    const ArgList &Args,
                    DiagnosticsEngine &Diags) {
  // Handle case without a `no-*` flag.
  if (!NegOpt)
    return Args.hasArg(Opt);

  // Handle case with a `no-*` flag.
  return Args.hasFlagAsOptional(Opt, *NegOpt);
}
```

It's possible you'll need to split up `OPTION_WITH_MARSHALLING` into two disjoint lists of options:
- The list of options that can't be negated.
- The list of options that can be negated, calling a different macro that adds macro arguments for the `CANCEL_ID` and `CANCEL_SPELLING`. For the denormalizer you might also need `CANCEL_VALUE`.
- Note: the negating options themselves wouldn't be visited in either list.
- Note: the (de)normalizer APIs would ideally work naturally for something like `-farg=val1` vs. `-farg=val2` vs. `-fno-arg`.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3931-3932
   if (((FLAGS)&options::CC1Option) &&                                          \
       (ALWAYS_EMIT || EXTRACTOR(this->KEYPATH) != DEFAULT_VALUE)) {            \
-    if (Option::KIND##Class == Option::FlagClass) {                            \
-      Args.push_back(SPELLING);                                                \
-    }                                                                          \
-    if (Option::KIND##Class == Option::SeparateClass) {                        \
-      Args.push_back(SPELLING);                                                \
-      DENORMALIZER(Args, SA, TABLE_INDEX, EXTRACTOR(this->KEYPATH));           \
-    }                                                                          \
+    DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX, EXTRACTOR(this->KEYPATH));   \
   }
----------------
I realize this commit doesn't introduce it, but it seems unfortunate to call `EXTRACTOR` twice. Maybe in a follow-up or prep commit you can fix that... maybe something like this?
```
  if ((FLAGS)&options::CC1Option) {
    const auto &Extracted = EXTRACTOR(this->KEYPATH);
    if (ALWAYS_EMIT || Extracted != DEFAULT_VALUE)
      DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX, EXTRACTOR(this->KEYPATH));
  }
```


================
Comment at: llvm/include/llvm/Option/OptParser.td:171
+  bit ShouldAlwaysEmit = 1;
+  code Normalizer ="normalizeBooleanTrueFlag<OPT_"#neg_name#">";
+  code Denormalizer = "denormalizeBooleanFlag<true>";
----------------
Nit: missing a space in `... Normalizer ="norma...`; same typo repeats below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83071