This is an archive of the discontinued LLVM Phabricator instance.

[clang][cli] Generate and round-trip Diagnostic options
ClosedPublic

Authored by jansvoboda11 on Feb 8 2021, 9:22 AM.

Details

Summary

This patch implements generation of remaining diagnostic options and tests it by performing parse-generate-parse round trip.

Diff Detail

Event Timeline

jansvoboda11 created this revision.Feb 8 2021, 9:22 AM
jansvoboda11 requested review of this revision.Feb 8 2021, 9:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 8 2021, 9:22 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
dexonsmith requested changes to this revision.Feb 8 2021, 11:26 AM
dexonsmith added inline comments.
clang/lib/Frontend/CompilerInvocation.cpp
745

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.

2230–2240

Does this mean that if some client is programmatically modifying the Warnings array (which is what CompilerInstance reads) they need to also update WarningsAsWritten array identically to get command-line generation to work?

If so, then this is a bit non-obvious / error-prone. Maybe these can encapsulated together:

struct DiagnosticArg {
  std::string Name;
  std::string AsWritten;
};
std::vector<DiagnosticArg> Warnings;
std::vector<DiagnosticArg> Remarks;
This revision now requires changes to proceed.Feb 8 2021, 11:26 AM

Remove the need for {Warnings,Remarks}AsWritten field

Got rid of {Warnings,Remarks}AsWritten entirely.

clang/lib/Frontend/CompilerInvocation.cpp
745

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.

2230–2240

I double-checked and managed to get rid of this entirely, see the other comment.

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?

dexonsmith accepted this revision.Feb 9 2021, 1:56 PM

LGTM!

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

clang/lib/Frontend/CompilerInvocation.cpp
745

Great!

This revision is now accepted and ready to land.Feb 9 2021, 1:56 PM
This revision was landed with ongoing or failed builds.Feb 10 2021, 3:45 AM
This revision was automatically updated to reflect the committed changes.