Skip to content

Template Diagnostic Improvements #99933

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

Template Diagnostic Improvements #99933

merged 4 commits into from
Jul 23, 2024

Conversation

bradenhelmer
Copy link
Contributor

@bradenhelmer bradenhelmer commented Jul 22, 2024

It turns out SemaTemplate handles this type of diagnostic already, however when template gets encountered, it never gets parsed as a possible statement or declaration, only as an expression.

Would like some feedback as I am unsure if this is the right fix. Thanks

Fixes #17959.

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

llvmbot commented Jul 22, 2024

@llvm/pr-subscribers-clang

Author: Braden Helmer (bradenhelmer)

Changes

This is for #17959.

It turns out SemaTemplate handles this type of diagnostic already, however when template gets encountered, it never gets parsed as a possible statement or declaration, only as an expression.

Would like some feedback as I am unsure if this is the right fix. Thanks


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

2 Files Affected:

  • (modified) clang/lib/Parse/ParseStmt.cpp (+9)
  • (modified) clang/test/Parser/cxx-template-decl.cpp (+4)
diff --git a/clang/lib/Parse/ParseStmt.cpp b/clang/lib/Parse/ParseStmt.cpp
index 22d38adc28ebe..bdb3fc051d0b3 100644
--- a/clang/lib/Parse/ParseStmt.cpp
+++ b/clang/lib/Parse/ParseStmt.cpp
@@ -299,6 +299,15 @@ StmtResult Parser::ParseStatementOrDeclarationAfterAttributes(
     goto Retry;
   }
 
+  case tok::kw_template: {
+    SourceLocation DeclEnd;
+    ParsedAttributes Attrs(AttrFactory);
+    ParseTemplateDeclarationOrSpecialization(DeclaratorContext::Block, DeclEnd,
+                                             Attrs,
+                                             getAccessSpecifierIfPresent());
+    return StmtError();
+  }
+
   case tok::kw_case:                // C99 6.8.1: labeled-statement
     return ParseCaseStatement(StmtCtx);
   case tok::kw_default:             // C99 6.8.1: labeled-statement
diff --git a/clang/test/Parser/cxx-template-decl.cpp b/clang/test/Parser/cxx-template-decl.cpp
index 734438069b9ae..5a2d1cec9ad31 100644
--- a/clang/test/Parser/cxx-template-decl.cpp
+++ b/clang/test/Parser/cxx-template-decl.cpp
@@ -297,3 +297,7 @@ namespace PR46231 {
   template<> int; // expected-error {{declaration does not declare anything}}
   template<int> int; // expected-error {{declaration does not declare anything}}
 }
+
+namespace NoTemplateInBlockScope {
+  void foo() { template <typename> int i; } // expected-error {{templates can only be declared in namespace or class scope}}
+}

Copy link
Contributor

@mizvekov mizvekov left a comment

Choose a reason for hiding this comment

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

Hi.

The change looks good.

There are some other examples of bad diagnostics in that issue, and it would be ideal if they are preserved in case this PR closes that issue.

Please also add an entry to clang/docs/ReleaseNotes.rst.

@bradenhelmer
Copy link
Contributor Author

This is handling the other non-local case now, and the release notes have been updated. The local class case was already being handled.

Copy link
Contributor

@mizvekov mizvekov left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks!

Please give it a couple of days before merging, in case anyone else wants to chime in.

@bradenhelmer bradenhelmer requested a review from Endilll as a code owner July 22, 2024 22:32
Copy link
Contributor

@mizvekov mizvekov left a comment

Choose a reason for hiding this comment

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

These updated test changes still LGTM.

Copy link
Contributor

@cor3ntin cor3ntin left a comment

Choose a reason for hiding this comment

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

LGTM.

@bradenhelmer
Copy link
Contributor Author

I don't have write access, if someone could merge that would be great!

@cor3ntin cor3ntin merged commit ef7d46c into llvm:main Jul 23, 2024
6 of 8 checks passed
@bradenhelmer bradenhelmer deleted the 17959 branch July 23, 2024 11:40
yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
It turns out `SemaTemplate` handles this type of diagnostic already,
however when template gets encountered, it never gets parsed as a
possible statement or declaration, only as an expression.

Fixes #17959.
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.

Diagnostics for template keywords showing up inside local classes aren't good
4 participants