Skip to content

[Clang] Fix : More Detailed "No expected directives found" #78338

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 23 commits into from
Feb 7, 2024
Merged

[Clang] Fix : More Detailed "No expected directives found" #78338

merged 23 commits into from
Feb 7, 2024

Conversation

Sh0g0-1758
Copy link
Member

@Sh0g0-1758 Sh0g0-1758 commented Jan 16, 2024

Fixes: #58290

PR SUMMARY: "Updated the error message to use the proper prefix when no expected directives are found by changing the hard coded expected in the message to a dynamic value in two error messages"

Copy link

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@llvmbot llvmbot added clang Clang issues not falling into any other category clangd clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jan 16, 2024
@llvmbot
Copy link
Member

llvmbot commented Jan 16, 2024

@llvm/pr-subscribers-clang

Author: Shourya Goel (Sh0g0-1758)

Changes

This pull request aims to solve the issue mentioned here
I have made the necessary changes to the best of my knowledge and updated tests for the same.
Please do suggest changes in it.


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

9 Files Affected:

  • (modified) clang-tools-extra/clangd/test/indexer.test (+1-1)
  • (modified) clang/include/clang/Basic/Diagnostic.h (+3-1)
  • (modified) clang/include/clang/Basic/DiagnosticFrontendKinds.td (+1-1)
  • (modified) clang/lib/AST/ASTContext.cpp (+1-1)
  • (modified) clang/lib/Frontend/VerifyDiagnosticConsumer.cpp (+4-1)
  • (modified) clang/test/ARCMT/verify.m (+1-1)
  • (modified) clang/test/Frontend/verify.c (+1-1)
  • (modified) clang/test/Frontend/verify2.c (+1-1)
  • (modified) clang/test/Frontend/verify3.c (+2-2)
diff --git a/clang-tools-extra/clangd/test/indexer.test b/clang-tools-extra/clangd/test/indexer.test
index 2f01f6c557a7de..213d33f8e2d6a5 100644
--- a/clang-tools-extra/clangd/test/indexer.test
+++ b/clang-tools-extra/clangd/test/indexer.test
@@ -5,5 +5,5 @@
 # `.ii` file and `-verify` triggers extra diagnostics generation. Clangd should
 # strip those.
 # RUN: clangd-indexer %t.cpp -- -Xclang -verify --save-temps -- 2>&1 | FileCheck %s
-# CHECK-NOT: error: no expected directives found: consider use of 'expected-no-diagnostics'
+# CHECK-NOT: error: no expected directives found: consider use of {{.*}}-no-diagnostics
 # RUN: not ls %t.ii
diff --git a/clang/include/clang/Basic/Diagnostic.h b/clang/include/clang/Basic/Diagnostic.h
index 0c7836c2ea569c..a185c75e418b58 100644
--- a/clang/include/clang/Basic/Diagnostic.h
+++ b/clang/include/clang/Basic/Diagnostic.h
@@ -9,7 +9,9 @@
 /// \file
 /// Defines the Diagnostic-related interfaces.
 //
-//===----------------------------------------------------------------------===//
+//===----------------------------------------------------------------------===//]
+
+// look into this file as well
 
 #ifndef LLVM_CLANG_BASIC_DIAGNOSTIC_H
 #define LLVM_CLANG_BASIC_DIAGNOSTIC_H
diff --git a/clang/include/clang/Basic/DiagnosticFrontendKinds.td b/clang/include/clang/Basic/DiagnosticFrontendKinds.td
index 568000106a84dc..42d2767af6b708 100644
--- a/clang/include/clang/Basic/DiagnosticFrontendKinds.td
+++ b/clang/include/clang/Basic/DiagnosticFrontendKinds.td
@@ -179,7 +179,7 @@ def err_verify_invalid_no_diags : Error<
     "%select{expected|'expected-no-diagnostics'}0 directive cannot follow "
     "%select{'expected-no-diagnostics' directive|other expected directives}0">;
 def err_verify_no_directives : Error<
-    "no expected directives found: consider use of 'expected-no-diagnostics'">;
+    "no expected directives found: consider use of '%0'-no-diagnostics">;
 def err_verify_nonconst_addrspace : Error<
   "qualifier 'const' is needed for variables in address space '%0'">;
 
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index b60dcfaabfd1a4..e3b2f1f48ed1e2 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -1404,7 +1404,7 @@ void ASTContext::InitBuiltinTypes(const TargetInfo &Target,
     getTranslationUnitDecl()->addDecl(MSGuidTagDecl);
   }
 }
-
+// maybe change here also.
 DiagnosticsEngine &ASTContext::getDiagnostics() const {
   return SourceMgr.getDiagnostics();
 }
diff --git a/clang/lib/Frontend/VerifyDiagnosticConsumer.cpp b/clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
index 8a3d2286cd168c..30cd7f47a21c1f 100644
--- a/clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
+++ b/clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
@@ -1098,7 +1098,10 @@ void VerifyDiagnosticConsumer::CheckDiagnostics() {
     // Produce an error if no expected-* directives could be found in the
     // source file(s) processed.
     if (Status == HasNoDirectives) {
-      Diags.Report(diag::err_verify_no_directives).setForceEmit();
+      // change here
+      std::string directives =
+          *Diags.getDiagnosticOptions().VerifyPrefixes.begin();
+      Diags.Report(diag::err_verify_no_directives).setForceEmit() << directives;
       ++NumErrors;
       Status = HasNoDirectivesReported;
     }
diff --git a/clang/test/ARCMT/verify.m b/clang/test/ARCMT/verify.m
index 7d245fe80575e5..13fd599f530fbe 100644
--- a/clang/test/ARCMT/verify.m
+++ b/clang/test/ARCMT/verify.m
@@ -11,7 +11,7 @@
 #error
 // expected-error@-1 {{}}
 
-//      CHECK: error: no expected directives found: consider use of 'expected-no-diagnostics'
+//      CHECK: error: no expected directives found: consider use of {{.*}}-no-diagnostics
 // CHECK-NEXT: error: 'expected-error' diagnostics seen but not expected:
 // CHECK-NEXT:   (frontend): error reading '{{.*}}verify.m.tmp.invalid'
 // CHECK-NEXT: 2 errors generated.
diff --git a/clang/test/Frontend/verify.c b/clang/test/Frontend/verify.c
index 221b715c19e416..657a9d3f2bf6bb 100644
--- a/clang/test/Frontend/verify.c
+++ b/clang/test/Frontend/verify.c
@@ -111,7 +111,7 @@ unexpected b; // expected-error@33 1-1 {{unknown type}}
 #if 0
 // RUN: not %clang_cc1 -verify %t.invalid 2>&1 | FileCheck -check-prefix=CHECK6 %s
 
-//      CHECK6: error: no expected directives found: consider use of 'expected-no-diagnostics'
+//      CHECK6: error: no expected directives found: consider use of {{.*}}-no-diagnostics
 // CHECK6-NEXT: error: 'expected-error' diagnostics seen but not expected:
 // CHECK6-NEXT:   (frontend): error reading '{{.*}}verify.c.tmp.invalid'
 // CHECK6-NEXT: 2 errors generated.
diff --git a/clang/test/Frontend/verify2.c b/clang/test/Frontend/verify2.c
index debaeb6b498678..77510c8d9b4940 100644
--- a/clang/test/Frontend/verify2.c
+++ b/clang/test/Frontend/verify2.c
@@ -12,7 +12,7 @@
 #if 0
 // expected-error {{should be ignored}}
 
-//      CHECK: error: no expected directives found: consider use of 'expected-no-diagnostics'
+//      CHECK: error: no expected directives found: consider use of {{.*}}-no-diagnostics
 // CHECK-NEXT: error: 'expected-error' diagnostics seen but not expected:
 // CHECK-NEXT:   Line 5: header
 // CHECK-NEXT:   Line 10: source
diff --git a/clang/test/Frontend/verify3.c b/clang/test/Frontend/verify3.c
index e414c7370fad77..8924619592f583 100644
--- a/clang/test/Frontend/verify3.c
+++ b/clang/test/Frontend/verify3.c
@@ -28,7 +28,7 @@
 #ifdef TEST3
 // no directives
 
-//      CHECK3: error: no expected directives found: consider use of 'expected-no-diagnostics'
+//      CHECK3: error: no expected directives found: consider use of {{.*}}-no-diagnostics
 // CHECK3-NEXT: 1 error generated.
 #endif
 
@@ -37,5 +37,5 @@
 #warning X
 // expected-warning@-1 {{X}}
 
-// CHECK4-NOT: error: no expected directives found: consider use of 'expected-no-diagnostics'
+// CHECK4-NOT: error: no expected directives found: consider use of {{.*}}-no-diagnostics
 #endif

@llvmbot
Copy link
Member

llvmbot commented Jan 16, 2024

@llvm/pr-subscribers-clangd

Author: Shourya Goel (Sh0g0-1758)

Changes

This pull request aims to solve the issue mentioned here
I have made the necessary changes to the best of my knowledge and updated tests for the same.
Please do suggest changes in it.


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

9 Files Affected:

  • (modified) clang-tools-extra/clangd/test/indexer.test (+1-1)
  • (modified) clang/include/clang/Basic/Diagnostic.h (+3-1)
  • (modified) clang/include/clang/Basic/DiagnosticFrontendKinds.td (+1-1)
  • (modified) clang/lib/AST/ASTContext.cpp (+1-1)
  • (modified) clang/lib/Frontend/VerifyDiagnosticConsumer.cpp (+4-1)
  • (modified) clang/test/ARCMT/verify.m (+1-1)
  • (modified) clang/test/Frontend/verify.c (+1-1)
  • (modified) clang/test/Frontend/verify2.c (+1-1)
  • (modified) clang/test/Frontend/verify3.c (+2-2)
diff --git a/clang-tools-extra/clangd/test/indexer.test b/clang-tools-extra/clangd/test/indexer.test
index 2f01f6c557a7de..213d33f8e2d6a5 100644
--- a/clang-tools-extra/clangd/test/indexer.test
+++ b/clang-tools-extra/clangd/test/indexer.test
@@ -5,5 +5,5 @@
 # `.ii` file and `-verify` triggers extra diagnostics generation. Clangd should
 # strip those.
 # RUN: clangd-indexer %t.cpp -- -Xclang -verify --save-temps -- 2>&1 | FileCheck %s
-# CHECK-NOT: error: no expected directives found: consider use of 'expected-no-diagnostics'
+# CHECK-NOT: error: no expected directives found: consider use of {{.*}}-no-diagnostics
 # RUN: not ls %t.ii
diff --git a/clang/include/clang/Basic/Diagnostic.h b/clang/include/clang/Basic/Diagnostic.h
index 0c7836c2ea569c..a185c75e418b58 100644
--- a/clang/include/clang/Basic/Diagnostic.h
+++ b/clang/include/clang/Basic/Diagnostic.h
@@ -9,7 +9,9 @@
 /// \file
 /// Defines the Diagnostic-related interfaces.
 //
-//===----------------------------------------------------------------------===//
+//===----------------------------------------------------------------------===//]
+
+// look into this file as well
 
 #ifndef LLVM_CLANG_BASIC_DIAGNOSTIC_H
 #define LLVM_CLANG_BASIC_DIAGNOSTIC_H
diff --git a/clang/include/clang/Basic/DiagnosticFrontendKinds.td b/clang/include/clang/Basic/DiagnosticFrontendKinds.td
index 568000106a84dc..42d2767af6b708 100644
--- a/clang/include/clang/Basic/DiagnosticFrontendKinds.td
+++ b/clang/include/clang/Basic/DiagnosticFrontendKinds.td
@@ -179,7 +179,7 @@ def err_verify_invalid_no_diags : Error<
     "%select{expected|'expected-no-diagnostics'}0 directive cannot follow "
     "%select{'expected-no-diagnostics' directive|other expected directives}0">;
 def err_verify_no_directives : Error<
-    "no expected directives found: consider use of 'expected-no-diagnostics'">;
+    "no expected directives found: consider use of '%0'-no-diagnostics">;
 def err_verify_nonconst_addrspace : Error<
   "qualifier 'const' is needed for variables in address space '%0'">;
 
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index b60dcfaabfd1a4..e3b2f1f48ed1e2 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -1404,7 +1404,7 @@ void ASTContext::InitBuiltinTypes(const TargetInfo &Target,
     getTranslationUnitDecl()->addDecl(MSGuidTagDecl);
   }
 }
-
+// maybe change here also.
 DiagnosticsEngine &ASTContext::getDiagnostics() const {
   return SourceMgr.getDiagnostics();
 }
diff --git a/clang/lib/Frontend/VerifyDiagnosticConsumer.cpp b/clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
index 8a3d2286cd168c..30cd7f47a21c1f 100644
--- a/clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
+++ b/clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
@@ -1098,7 +1098,10 @@ void VerifyDiagnosticConsumer::CheckDiagnostics() {
     // Produce an error if no expected-* directives could be found in the
     // source file(s) processed.
     if (Status == HasNoDirectives) {
-      Diags.Report(diag::err_verify_no_directives).setForceEmit();
+      // change here
+      std::string directives =
+          *Diags.getDiagnosticOptions().VerifyPrefixes.begin();
+      Diags.Report(diag::err_verify_no_directives).setForceEmit() << directives;
       ++NumErrors;
       Status = HasNoDirectivesReported;
     }
diff --git a/clang/test/ARCMT/verify.m b/clang/test/ARCMT/verify.m
index 7d245fe80575e5..13fd599f530fbe 100644
--- a/clang/test/ARCMT/verify.m
+++ b/clang/test/ARCMT/verify.m
@@ -11,7 +11,7 @@
 #error
 // expected-error@-1 {{}}
 
-//      CHECK: error: no expected directives found: consider use of 'expected-no-diagnostics'
+//      CHECK: error: no expected directives found: consider use of {{.*}}-no-diagnostics
 // CHECK-NEXT: error: 'expected-error' diagnostics seen but not expected:
 // CHECK-NEXT:   (frontend): error reading '{{.*}}verify.m.tmp.invalid'
 // CHECK-NEXT: 2 errors generated.
diff --git a/clang/test/Frontend/verify.c b/clang/test/Frontend/verify.c
index 221b715c19e416..657a9d3f2bf6bb 100644
--- a/clang/test/Frontend/verify.c
+++ b/clang/test/Frontend/verify.c
@@ -111,7 +111,7 @@ unexpected b; // expected-error@33 1-1 {{unknown type}}
 #if 0
 // RUN: not %clang_cc1 -verify %t.invalid 2>&1 | FileCheck -check-prefix=CHECK6 %s
 
-//      CHECK6: error: no expected directives found: consider use of 'expected-no-diagnostics'
+//      CHECK6: error: no expected directives found: consider use of {{.*}}-no-diagnostics
 // CHECK6-NEXT: error: 'expected-error' diagnostics seen but not expected:
 // CHECK6-NEXT:   (frontend): error reading '{{.*}}verify.c.tmp.invalid'
 // CHECK6-NEXT: 2 errors generated.
diff --git a/clang/test/Frontend/verify2.c b/clang/test/Frontend/verify2.c
index debaeb6b498678..77510c8d9b4940 100644
--- a/clang/test/Frontend/verify2.c
+++ b/clang/test/Frontend/verify2.c
@@ -12,7 +12,7 @@
 #if 0
 // expected-error {{should be ignored}}
 
-//      CHECK: error: no expected directives found: consider use of 'expected-no-diagnostics'
+//      CHECK: error: no expected directives found: consider use of {{.*}}-no-diagnostics
 // CHECK-NEXT: error: 'expected-error' diagnostics seen but not expected:
 // CHECK-NEXT:   Line 5: header
 // CHECK-NEXT:   Line 10: source
diff --git a/clang/test/Frontend/verify3.c b/clang/test/Frontend/verify3.c
index e414c7370fad77..8924619592f583 100644
--- a/clang/test/Frontend/verify3.c
+++ b/clang/test/Frontend/verify3.c
@@ -28,7 +28,7 @@
 #ifdef TEST3
 // no directives
 
-//      CHECK3: error: no expected directives found: consider use of 'expected-no-diagnostics'
+//      CHECK3: error: no expected directives found: consider use of {{.*}}-no-diagnostics
 // CHECK3-NEXT: 1 error generated.
 #endif
 
@@ -37,5 +37,5 @@
 #warning X
 // expected-warning@-1 {{X}}
 
-// CHECK4-NOT: error: no expected directives found: consider use of 'expected-no-diagnostics'
+// CHECK4-NOT: error: no expected directives found: consider use of {{.*}}-no-diagnostics
 #endif

@Sh0g0-1758
Copy link
Member Author

Screenshot from 2024-01-17 02-39-09
Here you can see a working implementation of the asked feature.

@Sh0g0-1758
Copy link
Member Author

Sh0g0-1758 commented Jan 16, 2024

I have pushed code in this PR that aims to solve this issue also.

@Sh0g0-1758
Copy link
Member Author

Screenshot from 2024-01-17 02-48-15
Here is a working implementation of the second issue that I aim to solve.

Copy link
Collaborator

@asl asl left a comment

Choose a reason for hiding this comment

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

Ensure that PR does not contain unrelated things. See https://llvm.org/docs/Contributing.html#how-to-submit-a-patch for more information

@Sh0g0-1758
Copy link
Member Author

I have removed the unnecessary comments. @asl please do let me know whether I should submit different PRs for the issues. But I think that @tbaederr can review them both in this PR only. Also @asl , do let me know any other required changes.

@Sh0g0-1758
Copy link
Member Author

@asl, I have checked all the files I changed and I have removed the unrelated changes and the changes related to another issue. Really sorry for this, I hope you can overlook it since this is my very first PR in LLVM.

@Sh0g0-1758
Copy link
Member Author

@tbaederr, please review the changes.

@Sh0g0-1758 Sh0g0-1758 requested review from tbaederr and asl January 17, 2024 20:05
@Sh0g0-1758 Sh0g0-1758 requested a review from tbaederr January 18, 2024 16:22
…ert changes in only -verify= test cases in verify files
Copy link

github-actions bot commented Jan 19, 2024

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

@Sh0g0-1758
Copy link
Member Author

Sh0g0-1758 commented Jan 19, 2024

Please see the desired behavior in this screenshot :

Screenshot from 2024-01-20 00-23-12

With -verify= we get expected-no-diagnostics and with -verify=foo we get foo-no-diagnostics

@tbaederr and @asl please see the updated changes and suggest improvements in it if any.

@Sh0g0-1758 Sh0g0-1758 requested a review from tbaederr January 24, 2024 12:35
@Sh0g0-1758
Copy link
Member Author

@asl @tbaederr @AaronBallman, can you please review this PR and merge it if everything seems fine.

@@ -179,7 +179,7 @@ def err_verify_invalid_no_diags : Error<
"%select{expected|'expected-no-diagnostics'}0 directive cannot follow "
"%select{'expected-no-diagnostics' directive|other expected directives}0">;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks like it needs templating?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you please elaborate on this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think @jrtc27 is saying that this diagnostic should also change to use %0 similar to what you've done for err_verify_no_directives because it has the same issue with saying 'expected-no-diagnostics'.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see, that can be done easily but I think according to LLVM PR standards, its best to limit one PR for one issue. Maybe you can start it as a different issue and then I will submit a PR for the same.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is the same issue -- in both cases, it's a hard-coded expected-no-diagnostics that is incorrect in the presence of a different prefix being used. Because of that, I'd still recommend doing it as part of this PR instead of as a separate one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah really sorry about that. Since this was pending for a few days and I wanted to push more PRs for the other issues without making a separate branch and rebasing them, I got a little impatient. Will surely keep this thing in mind from now on.

Copy link
Contributor

Choose a reason for hiding this comment

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

I definitely sympathize with you wanting to push more than one PR at a time, but the way to do it is to follow GitHub flow, specifically a part of it that says you should work in branches, and create your PRs from then: https://docs.github.com/en/get-started/using-github/github-flow#create-a-branch. This way PRs that touch different code can go through as many rounds of review as needed, without blocking other PRs.

Nagging reviewers won't get you anywhere, or even worse, deter reviewers from your PRs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes I understand. I made a beginners mistake of not making a branch for the changes in this PR. Will surely keep that in mind from now on. Also I hope you can overlook my enthu, was not trying to nag any reviewers.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No worries about the extra pings (we'd rather see people "too" enthusiastic than disengaged!), just something to keep in mind for the future. :-)

The reason I think this diagnostic also needs to be updated is because of: https://godbolt.org/z/sEf4o8YY9 and https://godbolt.org/z/8oMWffoxY where it says 'expected-no-diagnostics' in the diagnostic message. I think those should also say 'foo-no-diagnostics' similar to how you've fixed it for the no expected directives found warning.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks a lot for looking over it.

I have made the required changes and added more tests for the same. Please take a look and suggest changes.

@Sh0g0-1758
Copy link
Member Author

Sh0g0-1758 commented Jan 29, 2024

gentle ping.

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

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

Thank you for the fix! Can you please add more description to what is being fixed and how to the PR summary (as that becomes the commit message when we land the changes)?

I don't think a release note is necessary because this isn't really a user-facing feature (it's a feature for developers of Clang), but the changes should come with some test coverage, probably in clang/test/Frontend/verify.c or a new test similar to it.

@@ -179,7 +179,7 @@ def err_verify_invalid_no_diags : Error<
"%select{expected|'expected-no-diagnostics'}0 directive cannot follow "
"%select{'expected-no-diagnostics' directive|other expected directives}0">;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think @jrtc27 is saying that this diagnostic should also change to use %0 similar to what you've done for err_verify_no_directives because it has the same issue with saying 'expected-no-diagnostics'.

Comment on lines 1101 to 1108
std::string Err_Directive;
if (Diags.getDiagnosticOptions().VerifyPrefixes.empty()) {
Err_Directive = "expected";
} else {
Err_Directive = *Diags.getDiagnosticOptions().VerifyPrefixes.begin();
}
Diags.Report(diag::err_verify_no_directives).setForceEmit()
<< Err_Directive;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
std::string Err_Directive;
if (Diags.getDiagnosticOptions().VerifyPrefixes.empty()) {
Err_Directive = "expected";
} else {
Err_Directive = *Diags.getDiagnosticOptions().VerifyPrefixes.begin();
}
Diags.Report(diag::err_verify_no_directives).setForceEmit()
<< Err_Directive;
std::string ErrDirective;
if (Diags.getDiagnosticOptions().VerifyPrefixes.empty())
ErrDirective = "expected";
else
ErrDirective = *Diags.getDiagnosticOptions().VerifyPrefixes.begin();
Diags.Report(diag::err_verify_no_directives).setForceEmit()
<< ErrDirective;

We might want to split this logic out into a helper function if we're also updating err_verify_invalid_no_diags at the same time.

Copy link
Member Author

Choose a reason for hiding this comment

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

Please let me know if we are doing this also in this PR. I think its best to raise a separate issue for the same as LLVM PR standards imply.

@Sh0g0-1758
Copy link
Member Author

Sh0g0-1758 commented Jan 31, 2024

Hey @AaronBallman, I have added a test for the same in clang/test/Frontend/verify.c and made the necessary changes as well. I have added a short PR SUMMARY to the top-level comment as you instructed. As for changes in other error messages, I don't think they come under this PR.

@asl asl removed their request for review February 3, 2024 00:36
@Sh0g0-1758
Copy link
Member Author

gentle ping. Please check the mergeability of this PR.

@Endilll
Copy link
Contributor

Endilll commented Feb 5, 2024

gentle ping. Please check the mergeability of this PR.

Friendly reminder of one ping per week policy.

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

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

Thank you, aside from some small suggestions, this LGTM

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@AaronBallman
Copy link
Collaborator

I believe the concerns from @asl and @tbaederr have been addressed and we're fine to land this, but I'll land it tomorrow to give them both a chance to weigh-in and to give precommit CI a chance to finish.

@AaronBallman AaronBallman merged commit 4e58f03 into llvm:main Feb 7, 2024
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 clangd
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"No expected directives found" error message is confusing if -verify=foo was passed
7 participants