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

[Daniel Grumberg via Phabricator via llvm-commits]

dang marked 5 inline comments as done.
dang added inline comments.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:150
 
-static llvm::Optional<unsigned> normalizeSimpleEnum(OptSpecifier Opt,
-                                                    unsigned TableIndex,
----------------
dexonsmith wrote:
> 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.
Yes it was accidental sorry about that.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:161-163
+  if (!Args.hasArg(PosOpt, NegOpt))
+    return None;
+  return Args.hasFlag(PosOpt, NegOpt);
----------------
dexonsmith wrote:
> 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`.
`NegOpt` is passed in via the template parameter, the only weird bit about it is that the option name (for example OPT_fno_experimental_pass_manager) is constructed in tablegen by hardcoding the OPT_ prefix. What is currently there supports the case where the positive option takes a value and the negative one doesn't by using different normalizer/denormalizer pairs for the positive and the negative option. The bad thing about the current setup is that both options have identical normalizers, but I felt that was less bad than splitting the list of options and using different macros. 


================
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));   \
   }
----------------
dexonsmith wrote:
> 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));
>   }
> ```
Yes I can do that of course. Although EXTRACTOR is meant to be very cheap and in most cases it expands to just `this->KEYPATH`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83071