Skip to content

[verify] Improve the error messages with multiple active prefixes #126068

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Feb 27, 2025

Conversation

Maetveis
Copy link
Contributor

@Maetveis Maetveis commented Feb 6, 2025

Multiple improvements to make the messages more concrete, actionable and less confusing when multiple prefixes are used in -verify=. The common theme among these was that prior to the patch all error messages would use the alphabetically first prefix, even if the error was associated with a different one.

  • Mention the actual expected but unseen directive: Prior to this change when reporting expected but unseen directive, the alphabetically first one would be used to report the error even if that's not the one present in the source. Reword the diagnostic if multiple prefixes are active and include the real spelling of the expected directive for each expected but not seen line in the output.

  • Reword the seen but not expected error message if multiple directives are active to avoid having to pick an arbitrary (the first) prefix for it.

  • Include the full spelling of the directive when reporting a directive following the no-diagnostics directive. For example "'foo-error' directive cannot follow 'foo-no-diagnostics' directive"

  • Use the first appearing -no-diagnostics directive, in the above message instead of the first one alphabetically.

The new wording

diagnostics with '(error|warning|remark|note)' severity seen but not expected

instead of

'-(error|warning|remark|note)' diagnostics seen but not expected

is only used when multiple prefixes are present, the error messages stay the same with a single prefix only.

Multiple improvements to make the messages more concrete, actionable
and less confusing when multiple prefixes are used in `-verify`.
The common theme among these was that prior to the patch all error
messages would use the alphabetically first prefix, even if the error
was associated with a different one.

- Mention the actual expected but unseen directive:
  Prior to this change when reporting expected but unseen directive, the
  alphabetically first one would be used to report the error even if that's
  not the one present in the source.
  Reword the diagnostic if multiple prefixes are active and include
  the real spelling of the expected directive for each expected but not seen
  line in the output.

- Reword the seen but not expected error message if multiple directives
  are active to avoid having to pick an arbitrary (the first) prefix
  for it.

- Include the full spelling of the directive when reporting a directive
  following the no-diagnostics directive. For example
  "'foo-error' directive cannot follow 'foo-no-diagnostics' directive"

- Use the first appearing `-no-diagnostics` directive, in the above
  message instead of the first one alphabetically.
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Feb 6, 2025
@Maetveis Maetveis requested a review from tbaederr February 6, 2025 13:45
@llvmbot
Copy link
Member

llvmbot commented Feb 6, 2025

@llvm/pr-subscribers-clang

Author: Mészáros Gergely (Maetveis)

Changes

Multiple improvements to make the messages more concrete, actionable and less confusing when multiple prefixes are used in -verify. The common theme among these was that prior to the patch all error messages would use the alphabetically first prefix, even if the error was associated with a different one.

  • Mention the actual expected but unseen directive: Prior to this change when reporting expected but unseen directive, the alphabetically first one would be used to report the error even if that's not the one present in the source. Reword the diagnostic if multiple prefixes are active and include the real spelling of the expected directive for each expected but not seen line in the output.

  • Reword the seen but not expected error message if multiple directives are active to avoid having to pick an arbitrary (the first) prefix for it.

  • Include the full spelling of the directive when reporting a directive following the no-diagnostics directive. For example "'foo-error' directive cannot follow 'foo-no-diagnostics' directive"

  • Use the first appearing -no-diagnostics directive, in the above message instead of the first one alphabetically.

The new wording
> diagnostics with '(error|warning|remark|note)' severity seen but not expected

instead of

> '<prefix>-(error|warning|remark|note)' diagnostics seen but not expected

is only used when multiple prefixes are present, the error messages stay the same with a single prefix only.


Full diff: https://github.com/llvm/llvm-project/pull/126068.diff

6 Files Affected:

  • (modified) clang/include/clang/Basic/DiagnosticFrontendKinds.td (+4-4)
  • (modified) clang/include/clang/Frontend/VerifyDiagnosticConsumer.h (+15-7)
  • (modified) clang/lib/Frontend/VerifyDiagnosticConsumer.cpp (+42-31)
  • (added) clang/test/Frontend/verify-mulptiple-prefixes.c (+48)
  • (modified) clang/test/Frontend/verify.c (+1-1)
  • (modified) clang/test/Frontend/verify3.c (+1-1)
diff --git a/clang/include/clang/Basic/DiagnosticFrontendKinds.td b/clang/include/clang/Basic/DiagnosticFrontendKinds.td
index f3593f5313340b4..5a05f6c4e3e30fa 100644
--- a/clang/include/clang/Basic/DiagnosticFrontendKinds.td
+++ b/clang/include/clang/Basic/DiagnosticFrontendKinds.td
@@ -175,11 +175,11 @@ def err_verify_invalid_content : Error<
 def err_verify_missing_regex : Error<
     "cannot find start of regex ('{{') in %0">;
 def err_verify_inconsistent_diags : Error<
-    "'%0' diagnostics %select{expected|seen}1 but not %select{seen|expected}1: "
-    "%2">;
+    "%select{|'%1-%2' }0diagnostics %select{with '%2' severity |}0"
+    "%select{expected|seen}3 but not %select{seen|expected}3: "
+    "%4">;
 def err_verify_invalid_no_diags : Error<
-    "%select{expected|'%0-no-diagnostics'}1 directive cannot follow "
-    "%select{'%0-no-diagnostics' directive|other expected directives}1">;
+    "'%0' directive cannot follow %select{'%2' directive|other expected directives}1">;
 def err_verify_no_directives : Error<
     "no expected directives found: consider use of '%0-no-diagnostics'">;
 def err_verify_nonconst_addrspace : Error<
diff --git a/clang/include/clang/Frontend/VerifyDiagnosticConsumer.h b/clang/include/clang/Frontend/VerifyDiagnosticConsumer.h
index ddfae2666c4c397..b4c613712ed9b46 100644
--- a/clang/include/clang/Frontend/VerifyDiagnosticConsumer.h
+++ b/clang/include/clang/Frontend/VerifyDiagnosticConsumer.h
@@ -44,8 +44,9 @@ class VerifyDiagnosticConsumer: public DiagnosticConsumer,
   public:
     static std::unique_ptr<Directive>
     create(bool RegexKind, SourceLocation DirectiveLoc,
-           SourceLocation DiagnosticLoc, bool MatchAnyFileAndLine,
-           bool MatchAnyLine, StringRef Text, unsigned Min, unsigned Max);
+           SourceLocation DiagnosticLoc, StringRef Spelling,
+           bool MatchAnyFileAndLine, bool MatchAnyLine, StringRef Text,
+           unsigned Min, unsigned Max);
 
   public:
     /// Constant representing n or more matches.
@@ -53,6 +54,7 @@ class VerifyDiagnosticConsumer: public DiagnosticConsumer,
 
     SourceLocation DirectiveLoc;
     SourceLocation DiagnosticLoc;
+    const std::string Spelling;
     const std::string Text;
     unsigned Min, Max;
     bool MatchAnyLine;
@@ -71,10 +73,11 @@ class VerifyDiagnosticConsumer: public DiagnosticConsumer,
 
   protected:
     Directive(SourceLocation DirectiveLoc, SourceLocation DiagnosticLoc,
-              bool MatchAnyFileAndLine, bool MatchAnyLine, StringRef Text,
-              unsigned Min, unsigned Max)
-        : DirectiveLoc(DirectiveLoc), DiagnosticLoc(DiagnosticLoc), Text(Text),
-          Min(Min), Max(Max), MatchAnyLine(MatchAnyLine || MatchAnyFileAndLine),
+              StringRef Spelling, bool MatchAnyFileAndLine, bool MatchAnyLine,
+              StringRef Text, unsigned Min, unsigned Max)
+        : DirectiveLoc(DirectiveLoc), DiagnosticLoc(DiagnosticLoc),
+          Spelling(Spelling), Text(Text), Min(Min), Max(Max),
+          MatchAnyLine(MatchAnyLine || MatchAnyFileAndLine),
           MatchAnyFileAndLine(MatchAnyFileAndLine) {
       assert(!DirectiveLoc.isInvalid() && "DirectiveLoc is invalid!");
       assert((!DiagnosticLoc.isInvalid() || MatchAnyLine) &&
@@ -106,6 +109,11 @@ class VerifyDiagnosticConsumer: public DiagnosticConsumer,
     HasOtherExpectedDirectives
   };
 
+  struct ParsingState {
+    DirectiveStatus Status;
+    std::string FirstNoDiagnosticsDirective;
+  };
+
   class MarkerTracker;
 
 private:
@@ -118,7 +126,7 @@ class VerifyDiagnosticConsumer: public DiagnosticConsumer,
   const LangOptions *LangOpts = nullptr;
   SourceManager *SrcManager = nullptr;
   unsigned ActiveSourceFiles = 0;
-  DirectiveStatus Status;
+  ParsingState State;
   ExpectedData ED;
 
   void CheckDiagnostics();
diff --git a/clang/lib/Frontend/VerifyDiagnosticConsumer.cpp b/clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
index 48330e936171810..443f991e1ad62f2 100644
--- a/clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
+++ b/clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
@@ -89,9 +89,10 @@ namespace {
 class StandardDirective : public Directive {
 public:
   StandardDirective(SourceLocation DirectiveLoc, SourceLocation DiagnosticLoc,
-                    bool MatchAnyFileAndLine, bool MatchAnyLine, StringRef Text,
-                    unsigned Min, unsigned Max)
-      : Directive(DirectiveLoc, DiagnosticLoc, MatchAnyFileAndLine,
+                    StringRef Spelling, bool MatchAnyFileAndLine,
+                    bool MatchAnyLine, StringRef Text, unsigned Min,
+                    unsigned Max)
+      : Directive(DirectiveLoc, DiagnosticLoc, Spelling, MatchAnyFileAndLine,
                   MatchAnyLine, Text, Min, Max) {}
 
   bool isValid(std::string &Error) override {
@@ -106,9 +107,9 @@ class StandardDirective : public Directive {
 class RegexDirective : public Directive {
 public:
   RegexDirective(SourceLocation DirectiveLoc, SourceLocation DiagnosticLoc,
-                 bool MatchAnyFileAndLine, bool MatchAnyLine, StringRef Text,
-                 unsigned Min, unsigned Max, StringRef RegexStr)
-      : Directive(DirectiveLoc, DiagnosticLoc, MatchAnyFileAndLine,
+                 StringRef Spelling, bool MatchAnyFileAndLine, bool MatchAnyLine,
+                 StringRef Text, unsigned Min, unsigned Max, StringRef RegexStr)
+      : Directive(DirectiveLoc, DiagnosticLoc, Spelling, MatchAnyFileAndLine,
                   MatchAnyLine, Text, Min, Max),
         Regex(RegexStr) {}
 
@@ -285,6 +286,7 @@ class ParseHelper
 // The information necessary to create a directive.
 struct UnattachedDirective {
   DirectiveList *DL = nullptr;
+  std::string Spelling;
   bool RegexKind = false;
   SourceLocation DirectivePos, ContentBegin;
   std::string Text;
@@ -299,8 +301,8 @@ void attachDirective(DiagnosticsEngine &Diags, const UnattachedDirective &UD,
                      bool MatchAnyLine = false) {
   // Construct new directive.
   std::unique_ptr<Directive> D = Directive::create(
-      UD.RegexKind, UD.DirectivePos, ExpectedLoc, MatchAnyFileAndLine,
-      MatchAnyLine, UD.Text, UD.Min, UD.Max);
+      UD.RegexKind, UD.DirectivePos, ExpectedLoc, UD.Spelling,
+      MatchAnyFileAndLine, MatchAnyLine, UD.Text, UD.Min, UD.Max);
 
   std::string Error;
   if (!D->isValid(Error)) {
@@ -408,7 +410,7 @@ static std::string DetailedErrorString(const DiagnosticsEngine &Diags) {
 /// Returns true if any valid directives were found.
 static bool ParseDirective(StringRef S, ExpectedData *ED, SourceManager &SM,
                            Preprocessor *PP, SourceLocation Pos,
-                           VerifyDiagnosticConsumer::DirectiveStatus &Status,
+                           VerifyDiagnosticConsumer::ParsingState &State,
                            VerifyDiagnosticConsumer::MarkerTracker &Markers) {
   DiagnosticsEngine &Diags = PP ? PP->getDiagnostics() : SM.getDiagnostics();
 
@@ -440,8 +442,9 @@ static bool ParseDirective(StringRef S, ExpectedData *ED, SourceManager &SM,
     StringRef DToken = PH.Match();
     PH.Advance();
 
-    // Default directive kind.
     UnattachedDirective D;
+    D.Spelling = DToken;
+    // Default directive kind.
     const char *KindStr = "string";
 
     // Parse the initial directive token in reverse so we can easily determine
@@ -482,19 +485,22 @@ static bool ParseDirective(StringRef S, ExpectedData *ED, SourceManager &SM,
       continue;
 
     if (NoDiag) {
-      if (Status == VerifyDiagnosticConsumer::HasOtherExpectedDirectives)
+      if (State.Status == VerifyDiagnosticConsumer::HasOtherExpectedDirectives) {
         Diags.Report(Pos, diag::err_verify_invalid_no_diags)
-            << DetailedErrorString(Diags) << /*IsExpectedNoDiagnostics=*/true;
-      else
-        Status = VerifyDiagnosticConsumer::HasExpectedNoDiagnostics;
+            << D.Spelling << /*IsExpectedNoDiagnostics=*/true;
+      } else if (State.Status != VerifyDiagnosticConsumer::HasExpectedNoDiagnostics) {
+        State.Status = VerifyDiagnosticConsumer::HasExpectedNoDiagnostics;
+        State.FirstNoDiagnosticsDirective = D.Spelling;
+      }
       continue;
     }
-    if (Status == VerifyDiagnosticConsumer::HasExpectedNoDiagnostics) {
+    if (State.Status == VerifyDiagnosticConsumer::HasExpectedNoDiagnostics) {
       Diags.Report(Pos, diag::err_verify_invalid_no_diags)
-          << DetailedErrorString(Diags) << /*IsExpectedNoDiagnostics=*/false;
+          << D.Spelling << /*IsExpectedNoDiagnostics=*/false
+          << State.FirstNoDiagnosticsDirective;
       continue;
     }
-    Status = VerifyDiagnosticConsumer::HasOtherExpectedDirectives;
+    State.Status = VerifyDiagnosticConsumer::HasOtherExpectedDirectives;
 
     // If a directive has been found but we're not interested
     // in storing the directive information, return now.
@@ -670,7 +676,7 @@ VerifyDiagnosticConsumer::VerifyDiagnosticConsumer(DiagnosticsEngine &Diags_)
     : Diags(Diags_), PrimaryClient(Diags.getClient()),
       PrimaryClientOwner(Diags.takeClient()),
       Buffer(new TextDiagnosticBuffer()), Markers(new MarkerTracker(Diags)),
-      Status(HasNoDirectives) {
+      State{HasNoDirectives, {}} {
   if (Diags.hasSourceManager())
     setSourceManager(Diags.getSourceManager());
 }
@@ -788,7 +794,7 @@ bool VerifyDiagnosticConsumer::HandleComment(Preprocessor &PP,
   // Fold any "\<EOL>" sequences
   size_t loc = C.find('\\');
   if (loc == StringRef::npos) {
-    ParseDirective(C, &ED, SM, &PP, CommentBegin, Status, *Markers);
+    ParseDirective(C, &ED, SM, &PP, CommentBegin, State, *Markers);
     return false;
   }
 
@@ -818,7 +824,7 @@ bool VerifyDiagnosticConsumer::HandleComment(Preprocessor &PP,
   }
 
   if (!C2.empty())
-    ParseDirective(C2, &ED, SM, &PP, CommentBegin, Status, *Markers);
+    ParseDirective(C2, &ED, SM, &PP, CommentBegin, State, *Markers);
   return false;
 }
 
@@ -843,8 +849,8 @@ static bool findDirectives(SourceManager &SM, FileID FID,
 
   Token Tok;
   Tok.setKind(tok::comment);
-  VerifyDiagnosticConsumer::DirectiveStatus Status =
-    VerifyDiagnosticConsumer::HasNoDirectives;
+  VerifyDiagnosticConsumer::ParsingState State = {
+      VerifyDiagnosticConsumer::HasNoDirectives, {}};
   while (Tok.isNot(tok::eof)) {
     RawLex.LexFromRawLexer(Tok);
     if (!Tok.is(tok::comment)) continue;
@@ -857,7 +863,7 @@ static bool findDirectives(SourceManager &SM, FileID FID,
 
     // Find first directive.
     if (ParseDirective(Comment, nullptr, SM, nullptr, Tok.getLocation(),
-                       Status, Markers))
+                       State, Markers))
       return true;
   }
   return false;
@@ -887,10 +893,10 @@ static unsigned PrintUnexpected(DiagnosticsEngine &Diags, SourceManager *SourceM
     OS << ": " << I->second;
   }
 
+  const bool IsSinglePrefix = Diags.getDiagnosticOptions().VerifyPrefixes.size() == 1;
   std::string Prefix = *Diags.getDiagnosticOptions().VerifyPrefixes.begin();
-  std::string KindStr = Prefix + "-" + Kind;
   Diags.Report(diag::err_verify_inconsistent_diags).setForceEmit()
-      << KindStr << /*Unexpected=*/true << OS.str();
+      << IsSinglePrefix << Prefix << Kind << /*Unexpected=*/true << OS.str();
   return std::distance(diag_begin, diag_end);
 }
 
@@ -902,6 +908,9 @@ static unsigned PrintExpected(DiagnosticsEngine &Diags,
   if (DL.empty())
     return 0;
 
+  const bool IsSinglePrefix =
+      Diags.getDiagnosticOptions().VerifyPrefixes.size() == 1;
+
   SmallString<256> Fmt;
   llvm::raw_svector_ostream OS(Fmt);
   for (const auto *D : DL) {
@@ -917,13 +926,14 @@ static unsigned PrintExpected(DiagnosticsEngine &Diags,
       OS << " (directive at "
          << SourceMgr.getFilename(D->DirectiveLoc) << ':'
          << SourceMgr.getPresumedLineNumber(D->DirectiveLoc) << ')';
+    if (!IsSinglePrefix)
+      OS << " \'" << D->Spelling << '\'';
     OS << ": " << D->Text;
   }
 
   std::string Prefix = *Diags.getDiagnosticOptions().VerifyPrefixes.begin();
-  std::string KindStr = Prefix + "-" + Kind;
   Diags.Report(diag::err_verify_inconsistent_diags).setForceEmit()
-      << KindStr << /*Unexpected=*/false << OS.str();
+      << IsSinglePrefix << Prefix << Kind << /*Unexpected=*/false << OS.str();
   return DL.size();
 }
 
@@ -1109,11 +1119,11 @@ void VerifyDiagnosticConsumer::CheckDiagnostics() {
   if (SrcManager) {
     // Produce an error if no expected-* directives could be found in the
     // source file(s) processed.
-    if (Status == HasNoDirectives) {
+    if (State.Status == HasNoDirectives) {
       Diags.Report(diag::err_verify_no_directives).setForceEmit()
           << DetailedErrorString(Diags);
       ++NumErrors;
-      Status = HasNoDirectivesReported;
+      State.Status = HasNoDirectivesReported;
     }
 
     // Check that the expected diagnostics occurred.
@@ -1145,12 +1155,13 @@ void VerifyDiagnosticConsumer::CheckDiagnostics() {
 std::unique_ptr<Directive> Directive::create(bool RegexKind,
                                              SourceLocation DirectiveLoc,
                                              SourceLocation DiagnosticLoc,
+                                             StringRef Spelling,
                                              bool MatchAnyFileAndLine,
                                              bool MatchAnyLine, StringRef Text,
                                              unsigned Min, unsigned Max) {
   if (!RegexKind)
     return std::make_unique<StandardDirective>(DirectiveLoc, DiagnosticLoc,
-                                               MatchAnyFileAndLine,
+                                               Spelling, MatchAnyFileAndLine,
                                                MatchAnyLine, Text, Min, Max);
 
   // Parse the directive into a regular expression.
@@ -1175,7 +1186,7 @@ std::unique_ptr<Directive> Directive::create(bool RegexKind,
     }
   }
 
-  return std::make_unique<RegexDirective>(DirectiveLoc, DiagnosticLoc,
+  return std::make_unique<RegexDirective>(DirectiveLoc, DiagnosticLoc, Spelling,
                                           MatchAnyFileAndLine, MatchAnyLine,
                                           Text, Min, Max, RegexStr);
 }
diff --git a/clang/test/Frontend/verify-mulptiple-prefixes.c b/clang/test/Frontend/verify-mulptiple-prefixes.c
new file mode 100644
index 000000000000000..a42de9cedc52574
--- /dev/null
+++ b/clang/test/Frontend/verify-mulptiple-prefixes.c
@@ -0,0 +1,48 @@
+// Test the diagnostic messages of -verify with multiple prefixes.
+// - Expected but not seen errors should contain the prefix of the directive
+// - Seen but not expected errors should not choose an arbitrary prefix
+// - "expected directive cannot follow '<prefix>-no-diagnostics'" should report an actual
+//    expected-no-diagnostics prefix present in the source.
+
+// RUN: not %clang_cc1 -verify=foo,bar %s 2>&1 | FileCheck %s --check-prefix=CHECK1
+// RUN: not %clang_cc1 -verify=bar,foo %s 2>&1 | FileCheck %s --check-prefix=CHECK1
+
+undefined_type x; // #1
+
+// foo-error{{there is no error here}}
+// bar-error{{error not seen}}
+// bar-note{{declared here}}
+// bar-error{{another error not seen}}
+// bar-error-re{{regex error{{}} not present}}
+
+// CHECK1: error: diagnostics with 'error' severity expected but not seen: 
+// CHECK1:   Line 12 'foo-error': there is no error here
+// CHECK1:   Line 13 'bar-error': error not seen
+// CHECK1:   Line 15 'bar-error': another error not seen
+// CHECK1:   Line 16 'bar-error-re': regex error{{{{[}][}]}} not present
+// CHECK1: error: diagnostics with 'error' severity seen but not expected: 
+// CHECK1:   Line 10: unknown type name 'undefined_type'
+// CHECK1: error: diagnostics with 'note' severity expected but not seen: 
+// CHECK1:   Line 14 'bar-note': declared here
+// CHECK1: 6 errors generated.
+
+// RUN: not %clang_cc1 -verify=baz,qux,quux %s 2>&1 | FileCheck %s --check-prefix=CHECK2
+
+// qux-no-diagnostics
+// baz-error@#1{{unknown type name 'undefined_type'}}
+// quux-no-diagnostics
+// qux-error-re@#1{{unknown type name 'undefined_type'}}
+
+// CHECK2: error: diagnostics with 'error' severity seen but not expected: 
+// CHECK2:   Line 10: unknown type name 'undefined_type'
+// CHECK2:   Line 32: 'baz-error' directive cannot follow 'qux-no-diagnostics' directive
+// CHECK2:   Line 34: 'qux-error-re' directive cannot follow 'qux-no-diagnostics' directive
+
+// RUN: not %clang_cc1 -verify=spam,eggs %s 2>&1 | FileCheck %s --check-prefix=CHECK3
+
+// eggs-error@#1{{unknown type name 'undefined_type'}}
+// spam-no-diagnostics
+
+// CHECK3: error: diagnostics with 'error' severity seen but not expected: 
+// CHECK3:   Line 44: 'spam-no-diagnostics' directive cannot follow other expected directives
+// CHECK3: 1 error generated.
diff --git a/clang/test/Frontend/verify.c b/clang/test/Frontend/verify.c
index afd1c7d6907e20b..4a0a6b6a23d9c2f 100644
--- a/clang/test/Frontend/verify.c
+++ b/clang/test/Frontend/verify.c
@@ -201,7 +201,7 @@ unexpected b; // expected-error@33 1-1 {{unknown type}}
 // foo-note {{}}
 
 //      CHECK11: error: 'foo-error' diagnostics seen but not expected:
-// CHECK11-NEXT:   Line 201: expected directive cannot follow 'foo-no-diagnostics' directive
+// CHECK11-NEXT:   Line 201: 'foo-note' directive cannot follow 'foo-no-diagnostics' directive
 // CHECK11-NEXT: 1 error generated.
 #endif
 
diff --git a/clang/test/Frontend/verify3.c b/clang/test/Frontend/verify3.c
index e414c7370fad772..6d3ab79ae7c91ca 100644
--- a/clang/test/Frontend/verify3.c
+++ b/clang/test/Frontend/verify3.c
@@ -8,7 +8,7 @@
 // expected-note {{}}
 
 //      CHECK1: error: 'expected-error' diagnostics seen but not expected:
-// CHECK1-NEXT:   Line 8: expected directive cannot follow 'expected-no-diagnostics' directive
+// CHECK1-NEXT:   Line 8: 'expected-note' directive cannot follow 'expected-no-diagnostics' directive
 // CHECK1-NEXT: 1 error generated.
 #endif
 

Copy link

github-actions bot commented Feb 6, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

@tbaederr
Copy link
Contributor

This all looks like welcome improvements to me, but I'll let someone else make the final call. There are currently C++ meetings though, so some people might not be as available.

@Maetveis
Copy link
Contributor Author

Gentle ping @AaronBallman @MaskRay.

@Maetveis
Copy link
Contributor Author

Ping @MaskRay @AaronBallman I thought you might be interested in reviewing since you reviewed https://reviews.llvm.org/D154688.

@Maetveis Maetveis requested a review from erichkeane February 26, 2025 06:42
Copy link
Collaborator

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a distinct improvement, and don't see anything to be concerned about in the code.

Aaron is away at the WG14 meeting this week so don't expect a review from him anytime soon. That said, please give others ~24 hrs before merging.

@Maetveis Maetveis merged commit d91e5c3 into llvm:main Feb 27, 2025
8 checks passed
@Maetveis Maetveis deleted the improve-verify-multiple-error-msgs branch February 27, 2025 17:04
joaosaffran pushed a commit to joaosaffran/llvm-project that referenced this pull request Mar 3, 2025
…vm#126068)

Multiple improvements to make the messages more concrete, actionable and
less confusing when multiple prefixes are used in `-verify=`. The common
theme among these was that prior to the patch all error messages would
use the alphabetically first prefix, even if the error was associated
with a different one.

- Mention the actual expected but unseen directive: Prior to this change
when reporting expected but unseen directive, the alphabetically first
one would be used to report the error even if that's not the one present
in the source. Reword the diagnostic if multiple prefixes are active and
include the real spelling of the expected directive for each expected
but not seen line in the output.

- Reword the seen but not expected error message if multiple directives
are active to avoid having to pick an arbitrary (the first) prefix for
it.

- Include the full spelling of the directive when reporting a directive
following the no-diagnostics directive. For example "'foo-error'
directive cannot follow 'foo-no-diagnostics' directive"

- Use the first appearing `-no-diagnostics` directive, in the above
message instead of the first one alphabetically.

The new wording
> diagnostics with '(error|warning|remark|note)' severity seen but not
expected

instead of

> '<prefix>-(error|warning|remark|note)' diagnostics seen but not
expected

is only used when multiple prefixes are present, the error messages stay
the same with a single prefix only.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants