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