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