jansvoboda11 added inline comments.
================
Comment at: llvm/utils/TableGen/OptParserEmitter.cpp:102-107
+ std::string getMacroName() const {
+ if (KeyPath.startswith("DiagnosticOpts."))
+ return (Twine("DIAG_") + MarshallingInfo::MacroName).str();
+
+ return MarshallingInfo::MacroName;
+ }
----------------
dexonsmith wrote:
> jansvoboda11 wrote:
> > dexonsmith wrote:
> > > This seems like a bit of a semantic layering violation. It'd be pretty
> > > unexpected if someone renamed `DiagnosticOpts` in clang that they'd have
> > > to update this code in llvm. Is there another way to solve this problem?
> > I don't like it either, but the alternatives I can think of are worse.
> >
> > We could add a `string MacroPrefix;` field to LLVM's `Option` class and
> > populate it in Clang's TableGen file:
> > 1. Via something like an `IsDiag` multiclass that we'd need to remember to
> > apply to each diagnostic option. I don't like it as it seems error prone
> > and introduces duplication.
> > 2. Put all diagnostic options into a single `let MacroPrefix = "DIAG_" in {
> > ... }` block. This removes the duplication, but doesn't ensure an option is
> > in that block iff it's a diagnostic option with `"DiagnosticOpts.*"`
> > keypath.
> > 3. More involved approach would be to duplicate the LLVM's `Option` and
> > related stuff in Clang. That would get us a place to put the custom
> > `KeyPath.startswith("DiagnosticOpts.")` logic and then forward to LLVM's
> > `Option` with the appropriate `MacroPrefix`.
> >
> > I'll think some more about it.
> Doing #1 + #2 seems like an okay tradeoff to me (looking back at the old
> diff, it seems like that's similar to what @dang originally implemented
> (except adding a more generic `MacroPrefix`), and it seems fairly clean /
> obvious to me).
>
> > [...] but doesn't ensure an option is in that block iff it's a diagnostic
> > option with "DiagnosticOpts.*" keypath.
>
> The reason I'm okay with this is that I'm having trouble envisioning how this
> would go wrong practice.
> - If someone adds somethings to `DiagnosticOptions`, they're likely to grep
> for how the adjacent field was defined / marshalled, duplicate the line, and
> modify it. I'm not seeing a likely path for them to copy/paste from a
> non-diagnostic option and/or miss adding this to the `let` block.
> - If someone accidentally adds something to the `let` block that isn't in
> `DiagnosticOptions`, they'll get a compiler error in `ParseDiagnosticArgs`.
>
> If you're still concerned, I wonder if there's a way to add a check in
> asserts builds that confirms that `ParseDiagnosticArgs` fills in
> `DiagnosticOptions` equivalently to how `createFromCommandLine` does? (and/or
> could the latter call the former as an implementation strategy?)
> - If someone adds somethings to `DiagnosticOptions`, they're likely to grep
> for how the adjacent field was defined / marshalled, duplicate the line, and
> modify it. I'm not seeing a likely path for them to copy/paste from a
> non-diagnostic option and/or miss adding this to the `let` block.
I think that's a fair assumption.
> - If someone accidentally adds something to the `let` block that isn't in
> `DiagnosticOptions`, they'll get a compiler error in `ParseDiagnosticArgs`.
That's right.
I think it's fine to move from the check in `OptParserEmitter` to a
`MacroPrefix` then.
I chose the `IsDiag` mixin as opposed to `let MacroPrefix = "_DIAG" in { ...
}`, as it doesn't require moving options around - cleaner diff.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D84673/new/
https://reviews.llvm.org/D84673
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
