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