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

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

jdenny added a comment.





================
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:
> jdenny wrote:
> > 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.
> >> 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)?
> 
> No, I'm saying that if we retain this as an error at all, then it needs better text. I'd prefer it not be an error.
> 
> > I see three reasons not to permit duplicate explicit prefixes:
> 
> I don't have a strong opinion, but in general, prohibiting duplicating command-line options hurts composability of command lines and makes scripting more difficult. That's a general statement (i.e., not tied to this use case), but as a result, I feel that should be the default unless a good reason (technical or otherwise) is presented.
> 
> In this case, checking for duplicates adds complexity to the implementation, and as far as I can tell, adds little value. Obviously, when writing checks, the author should check that they work. Moreover, the implementation will already complain if there are unmatched diagnostics, or if nothing matches, so the chance of accidentally mistyping the verify prefix seems low. 
> 
> > 1. Not permitting duplicates is consistent with FileCheck's --check-prefix[es].
> 
> I don't find this compelling. In part, this is because FileCheck can't complain about unmatched output (that wouldn't make sense), and so the chance of error with FileCheck is much higher.
> 
> > 2. If we change our mind, we can later relax the restriction without breaking backward compatibility, but we cannot go the other direction.
> 
> Not for this kind of option, really. This is a tool for Clang developers. If we found in the future that allowing duplicates where a large source of errors, we'd just change it.
> 
> > 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.
> 
> If someone else feels strongly about this, please speak up. I don't see this as worth the implementation complexity nor sufficient justification to override what I see as the best practice of allowing duplicate options.
> 
> > 
> > I'm not adamant about the current behavior, but I think we should consider these points before deciding.
> 
> Sure.
I'm not sure why, but Phabricator doesn't give me the menu option to quote your comment in full this time, so I'll copy and paste just the most important lines.

> No, I'm saying that if we retain this as an error at all, then it needs better text. I'd prefer it not be an error.

Does the text for spelling errors, which are diagnosed immediately before this, look good to you?

> In this case, checking for duplicates adds complexity to the implementation

But really not very much.

> FileCheck can't complain about unmatched output (that wouldn't make sense), and so the chance of error with FileCheck is much higher.

That's an important distinction I hadn't appreciated.

> This is a tool for Clang developers. If we found in the future that allowing duplicates where a large source of errors, we'd just change it.

That argument convinces me.  Unless someone objects before I get to it, I'll remove the restriction for duplicates.


https://reviews.llvm.org/D39694