[PATCH] D84673: [clang][cli] Port DiagnosticOpts to new option parsing system

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

dexonsmith 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;
+  }
----------------
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?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84673