[clang] Show verify prefix in error messages

[aaron.ballman]

I'm not opposed but I am wondering what the need is for this? (It'd be good to capture that information in the patch summary as well.) Some examples showing where this helps to correct the issue would be useful.

[tbaeder]

When passing a different prefix via -verify=foo, the error messages now say "error: 'foo-error' diagnostics seen but not expected", etc.

I'm often working in test files where two different prefixes are used and I'm always confused about which one of the two the error messages are talking about.

[aaron.ballman]

In D154688#4497967, @tbaeder wrote:

When passing a different prefix via -verify=foo, the error messages now say "error: 'foo-error' diagnostics seen but not expected", etc.

I'm often working in test files where two different prefixes are used and I'm always confused about which one of the two the error messages are talking about.

What I'm confused by is that we list the line numbers of the failures, so the prefix only seems like it's helpful in a case where two prefixes use the same message and the same severity on the same line. e.g., // foo-error {{whatever}} bar-error {{whatever}}. In the other cases, either the line number is different, or the severity is different, or the message is different which I thought was giving sufficient context.

[tbaeder]

In D154688#4498398, @aaron.ballman wrote:

In D154688#4497967, @tbaeder wrote:

When passing a different prefix via -verify=foo, the error messages now say "error: 'foo-error' diagnostics seen but not expected", etc.

I'm often working in test files where two different prefixes are used and I'm always confused about which one of the two the error messages are talking about.

What I'm confused by is that we list the line numbers of the failures, so the prefix only seems like it's helpful in a case where two prefixes use the same message and the same severity on the same line. e.g., // foo-error {{whatever}} bar-error {{whatever}}. In the other cases, either the line number is different, or the severity is different, or the message is different which I thought was giving sufficient context.

This is also reported as being on line 4:

// RUN: %clang_cc1 -fsyntax-only -fdiagnostics-print-source-range-info -verify=bar %s 2>&1


static_assert(true); // foo \
                     // bar-error {{failed}}

which is also the case if line 4 contained another foo-error {{failed}} which didn't trigger, leaving developers wondering which one it is.

[tbaeder]

Ping

[aaron.ballman]

In D154688#4499797, @tbaeder wrote:

In D154688#4498398, @aaron.ballman wrote:

In D154688#4497967, @tbaeder wrote:

When passing a different prefix via -verify=foo, the error messages now say "error: 'foo-error' diagnostics seen but not expected", etc.

I'm often working in test files where two different prefixes are used and I'm always confused about which one of the two the error messages are talking about.

What I'm confused by is that we list the line numbers of the failures, so the prefix only seems like it's helpful in a case where two prefixes use the same message and the same severity on the same line. e.g., // foo-error {{whatever}} bar-error {{whatever}}. In the other cases, either the line number is different, or the severity is different, or the message is different which I thought was giving sufficient context.

This is also reported as being on line 4:

// RUN: %clang_cc1 -fsyntax-only -fdiagnostics-print-source-range-info -verify=bar %s 2>&1


static_assert(true); // foo \
                     // bar-error {{failed}}

which is also the case if line 4 contained another foo-error {{failed}} which didn't trigger, leaving developers wondering which one it is.

Okay, we do that often enough in our tests (along with two diagnostics on the same line directly) that this seems pretty reasonable to me.

Can you add some test coverage where the prefix is not the default expected?

[MaskRay]

The change looks great: (from error: 'error' diagnostics seen but not expected: to error: 'expected-error' diagnostics seen but not expected).
When I read Clang tests, I was confused by what error meant. I eventually figured out -verify is special. The new diagnostic is clearer and more helpful.

[aaron.ballman]

Precommit CI found relevant failures that should be addressed.

[MaskRay]

clang/lib/Frontend/VerifyDiagnosticConsumer.cpp

881

The variables are immediately used. I think in this case our convention is omit the blank line (and in general I think we omit blank lines for many more cases, compared to some other projects (GNU, Linux kernel, etc)).

clang/test/Frontend/verify.c

159 ↗

(On Diff #543260)

This may need a comment explaining that this is not recognized as a directive.

[tbaeder]

clang/test/Frontend/verify.c

159 ↗

(On Diff #543260)

Not sure I follow, what isn't being recognized as a directive?

[tbaeder]

Ping

[tbaeder]

Ping

[aaron.ballman]

LGTM!

clang/lib/Frontend/VerifyDiagnosticConsumer.cpp

else {
       if (const FileEntry *File = SourceMgr->getFileEntryForID(
                                                 SourceMgr->getFileID(I->first)))
         OS << " File " << File->getName();
       OS << " Line " << SourceMgr->getPresumedLineNumber(I->first);
     }
     OS << ": " << I->second;
   }
 
+  std::string Prefix = *Diags.getDiagnosticOptions().VerifyPrefixes.begin();
+  std::string KindStr = Prefix + "-" + Kind;
+
   Diags.Report(diag::err_verify_inconsistent_diags).setForceEmit()
-    << Kind << /*Unexpected=*/true << OS.str();
+      << KindStr << /*Unexpected=*/true << OS.str();
   return std::distance(diag_begin, diag_end);
 }
 
 /// Takes a list of diagnostics that were expected to have been generated
 /// but were not and produces a diagnostic to the user from this.
 static unsigned PrintExpected(DiagnosticsEngine &Diags,
                               SourceManager &SourceMgr,
                               std::vector<Directive *> &DL, const char *Kind) {
else
       OS << " Line " << SourceMgr.getPresumedLineNumber(D->DiagnosticLoc);
     if (D->DirectiveLoc != D->DiagnosticLoc)
       OS << " (directive at "
          << SourceMgr.getFilename(D->DirectiveLoc) << ':'
          << SourceMgr.getPresumedLineNumber(D->DirectiveLoc) << ')';
     OS << ": " << D->Text;
   }
 
+  std::string Prefix = *Diags.getDiagnosticOptions().VerifyPrefixes.begin();
+  std::string KindStr = Prefix + "-" + Kind;
+
   Diags.Report(diag::err_verify_inconsistent_diags).setForceEmit()
-    << Kind << /*Unexpected=*/false << OS.str();
+      << KindStr << /*Unexpected=*/false << OS.str();
   return DL.size();
 }
 
 /// Determine whether two source locations come from the same file.
 static bool IsFromSameFile(SourceManager &SM, SourceLocation DirectiveLoc,
                            SourceLocation DiagnosticLoc) {
   while (DiagnosticLoc.isMacroID())
     DiagnosticLoc = SM.getImmediateMacroCallerLoc(DiagnosticLoc);