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

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

dexonsmith added inline comments.


================
Comment at: clang/include/clang/Driver/Options.td:1193-1195
+defm caret_diagnostics : BoolFOption<"caret-diagnostics",
+  "DiagnosticOpts->ShowCarets", DefaultsToTrue,
+  ChangedBy<NegFlag>, ResetBy<PosFlag>>, IsDiag;
----------------
There was one thing in the original patch that was a bit of sanity check for whether `IsDiag` was added to the wrong option: the `DiagnosticOpts->` part of the keypath was implicit. What do you think of adding that back? That would make the keypath here `ShowCarets`.

It highlights that diagnostics are special (since `DiagnosticOpts` is never mentioned in the file, and any mistakes could be found with a `grep`).


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3212
+
+#define DIAG_OPTION_WITH_MARSHALLING OPTION_WITH_MARSHALLING
+
----------------
Making `DiagnosticOpts->` implicit requires a little more code here:
```
#define DIAG_OPTION_WITH_MARSHALLING(                                          \
    PREFIX_TYPE, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,        \
    HELPTEXT, METAVAR, VALUES, SPELLING, ALWAYS_EMIT, KEYPATH, DEFAULT_VALUE,  \
    IMPLIED_CHECK, IMPLIED_VALUE, NORMALIZER, DENORMALIZER, MERGER, EXTRACTOR, \
    TABLE_INDEX)                                                               \
  GENERATE_OPTION_WITH_MARSHALLING(                                            \
      Args, SA, KIND, FLAGS, SPELLING, ALWAYS_EMIT, DiagnosticOpts->KEYPATH,   \
      DEFAULT_VALUE, IMPLIED_CHECK, IMPLIED_VALUE, DENORMALIZER, EXTRACTOR,    \
      TABLE_INDEX)
```
But I think it might be worth it for the sanity check.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84673