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