[PATCH] D39694: [VerifyDiagnosticConsumer] support -verify=<prefixes>

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

I think this is a good idea.



================
Comment at: include/clang/Driver/CC1Options.td:407
 def verify : Flag<["-"], "verify">,
-  HelpText<"Verify diagnostic output using comment directives">;
+  HelpText<"Similar to -verify=expected">;
 def verify_ignore_unexpected : Flag<["-"], "verify-ignore-unexpected">,
----------------
"Similar to" seems unfortunately vague. Can it say, "Equivalent to ..."?


================
Comment at: lib/Frontend/CompilerInvocation.cpp:1081
+                                [](char C){return !isAlphanumeric(C)
+                                                  && C!='-' && C!='_';});
+    if (BadChar != Prefix.end() || !isLetter(Prefix[0])) {
----------------
I'd prefer to have spaces around the != operators.


================
Comment at: lib/Frontend/CompilerInvocation.cpp:1095
+      if (Diags) {
+        Diags->Report(diag::err_drv_invalid_value) << "-verify=" << Prefix;
+        Diags->Report(diag::note_drv_verify_prefix_unique);
----------------
If this were going to be an error, then it should have error message that explains the problem (e.g., duplicate --verify prefix). I don't believe that we should make this an error (i.e., I think that we should allow duplicate --verify options with the same prefixes).


================
Comment at: lib/Frontend/VerifyDiagnosticConsumer.cpp:233
   // Return true if string literal is found.
   // When true, P marks begin-position of S in content.
+  bool Search(StringRef S, bool EnsureStartOfWord = false,
----------------
Please document here what FinishDirectiveWord means (and, while you're at it, documenting what EnsureStartOfWord means would be nice too).


================
Comment at: lib/Frontend/VerifyDiagnosticConsumer.cpp:370
+    bool NoDiag = false;
+    {
+      StringRef DType;
----------------
This block is to limit the scope of the DType StringRef? That doesn't seem worthwhile.


https://reviews.llvm.org/D39694