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

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

In https://reviews.llvm.org/D39694#944642, @hfinkel wrote:

> I think this is a good idea.


Thanks for the review.  I've replied to each comment and will make revisions after we agree on the behavioral issue you raised.



================
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">,
----------------
hfinkel wrote:
> "Similar to" seems unfortunately vague. Can it say, "Equivalent to ..."?
I agree I should have made it clearer.

"Equivalent to -verify=expected"  works if we decide that duplicate explicit prefixes are permitted, as you've suggested in a later comment.

With the current implementation, it should be "All occurrences together are equivalent to one occurrence of -verify=expected".  That is, I chose to permit multiple occurrences of -verify without explicit prefixes for backward compatibility, but I chose not to permit duplicate explicit prefixes for reasons I'll discuss in the other comment.

I'll clean up the documentation once we agree on the right behavior.




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


================
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);
----------------
hfinkel wrote:
> 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).
> If this were going to be an error, then it should have error message that explains the problem (e.g., duplicate --verify prefix).

Are you saying you prefer that to be in the error instead of the note (where it is now)?

> 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).

I see two reasons to permit duplicate explicit prefixes:

1. It simplifies the documentation some (see previous comment).

2. Typically, it's convenient to permit command-line options to be repeated so that groups of options can be collected in shell or make variables without having to worry about conflicts between groups.  On the other hand, I'm having trouble imagining that use case for -verify options, which I believe would normally appear directly in command lines in test source files.  Have you seen use cases (perhaps with FileCheck) where it would be useful?

I see three reasons not to permit duplicate explicit prefixes:

1. Not permitting duplicates is consistent with FileCheck's --check-prefix[es].

2. If we change our mind, we can later relax the restriction without breaking backward compatibility, but we cannot go the other direction.

3. Suppose a developer wants to extend an existing test case by adding new -verify prefixes to existing clang command lines that already uses many -verify prefixes.  If the developer accidentally duplicates an existing prefix, the test case surely will not behave as expected, but it should be easier to understand what has gone wrong if the compiler complains about duplicate prefixes.

I'm not adamant about the current behavior, but I think we should consider these points before deciding.


================
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,
----------------
hfinkel wrote:
> Please document here what FinishDirectiveWord means (and, while you're at it, documenting what EnsureStartOfWord means would be nice too).
Sure, I'll work on it.


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


https://reviews.llvm.org/D39694