[PATCH] D96274: [clang][cli] Generate and round-trip Diagnostic options

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

dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.

LGTM!

In D96274#2551206 <https://reviews.llvm.org/D96274#2551206>, @probinson wrote:

> I don't claim to be very familiar with this area but "round-trip" and "test" would make me think those bits should be in a lit test or unittest. As it is, it's not obvious what is functional and what is testing here.
> Possibly I am misunderstanding something fundamental about how these options work?

The idea is to eventually / soon turn `-round-trip-args` on by default in asserts builds. The option changes "parse `-cc1`" to "parse1 => generate1 => parse2 => generate2", returning the result of "parse2" (instead of "parse1") so that new options cannot be added without adding matching generate code, and comparing "generate1" against "generate2" to ensure generation is consistent / deterministic. Having it on by default in asserts builds will add coverage for all `-cc1` options used in any test.

Note that the per-option-group round-tripping is temporary, to enable some of this verification to land incrementally. The following patch combines it into a single high-level operation:
https://reviews.llvm.org/D96280



================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:745
+
+    A->render(Args, Rendered);
   }
----------------
jansvoboda11 wrote:
> dexonsmith wrote:
> > It's not obvious why this renders the args instead of calling `A->getAsString()` to get the literal command-line argument and putting it directly in DiagnosticsAsWritten. If that'll work, I suggest switching to that, since it's a bit more straightforward; if there's some reason you can't do that, please leave a comment explaining why.
> I removed `DiagnosticsAsWritten` completely.
> 
> Previously, the code looped over `A->getValues()`, which gave me the impression it handles a flag that might indeed have multiple values (e.g. `-Wsomething=a,b,c`). If the code only stored `{a,b,c}` into `Diagnostics`, `GenerateDiagnosticArgs` could never reconstruct the original argument.
> 
> However, after looking closely at all `-W` and `-R` options, this never happens and `A->getValues()` always has exactly one element. I've refactored the code and clarified the comment for the branch above.
Great!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96274