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