[clang] Refactor attr diagnostics to use %select #122473

emaxx-google · 2025-01-10T15:18:27Z

A cleanup follow-up to #118501 and #118567.

llvmbot · 2025-01-10T15:22:26Z

@llvm/pr-subscribers-clang

Author: Maksim Ivanov (emaxx-google)

Changes

A cleanup follow-up to #118567 and #118567.

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

8 Files Affected:

(modified) clang/examples/Attribute/Attribute.cpp (+5-4)
(modified) clang/examples/CallSuperAttribute/CallSuperAttrInfo.cpp (+3-2)
(modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+8-1)
(modified) clang/include/clang/Sema/ParsedAttr.h (+7)
(modified) clang/lib/Parse/ParseDecl.cpp (+3-2)
(modified) clang/lib/Sema/SemaDeclAttr.cpp (+6-6)
(modified) clang/lib/Sema/SemaSwift.cpp (+2-2)
(modified) clang/lib/Sema/SemaType.cpp (+5-4)

diff --git a/clang/examples/Attribute/Attribute.cpp b/clang/examples/Attribute/Attribute.cpp
index 3b90724ad22205..625f1645afbff6 100644
--- a/clang/examples/Attribute/Attribute.cpp
+++ b/clang/examples/Attribute/Attribute.cpp
@@ -42,8 +42,8 @@ struct ExampleAttrInfo : public ParsedAttrInfo {
                             const Decl *D) const override {
     // This attribute appertains to functions only.
     if (!isa<FunctionDecl>(D)) {
-      S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type_str)
-          << Attr << Attr.isRegularKeywordAttribute() << "functions";
+      S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type)
+          << Attr << Attr.isRegularKeywordAttribute() << ExpectedFunction;
       return false;
     }
     return true;
@@ -99,8 +99,9 @@ struct ExampleAttrInfo : public ParsedAttrInfo {
                             const Stmt *St) const override {
     // This attribute appertains to for loop statements only.
     if (!isa<ForStmt>(St)) {
-      S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type_str)
-          << Attr << Attr.isRegularKeywordAttribute() << "for loop statements";
+      S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type)
+          << Attr << Attr.isRegularKeywordAttribute()
+          << ExpectedForLoopStatement;
       return false;
     }
     return true;
diff --git a/clang/examples/CallSuperAttribute/CallSuperAttrInfo.cpp b/clang/examples/CallSuperAttribute/CallSuperAttrInfo.cpp
index 12d4c311586e6f..f206a84ab1311d 100644
--- a/clang/examples/CallSuperAttribute/CallSuperAttrInfo.cpp
+++ b/clang/examples/CallSuperAttribute/CallSuperAttrInfo.cpp
@@ -168,8 +168,9 @@ struct CallSuperAttrInfo : public ParsedAttrInfo {
                             const Decl *D) const override {
     const auto *TheMethod = dyn_cast_or_null<CXXMethodDecl>(D);
     if (!TheMethod || !TheMethod->isVirtual()) {
-      S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type_str)
-          << Attr << Attr.isRegularKeywordAttribute() << "virtual functions";
+      S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type)
+          << Attr << Attr.isRegularKeywordAttribute()
+          << ExpectedVirtualFunction;
       return false;
     }
     MarkedMethods.insert(TheMethod);
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index d4e897868f1a9a..e96e87b3e56558 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3799,7 +3799,14 @@ def warn_attribute_wrong_decl_type : Warning<
   "|types and namespaces"
   "|variables, functions and classes"
   "|kernel functions"
-  "|non-K&R-style functions}2">,
+  "|non-K&R-style functions"
+  "|for loop statements"
+  "|virtual functions"
+  "|parameters and implicit object parameters"
+  "|non-member functions"
+  "|functions, classes, or enumerations"
+  "|classes"
+  "|typedefs}2">,
   InGroup<IgnoredAttributes>;
 def err_attribute_wrong_decl_type : Error<warn_attribute_wrong_decl_type.Summary>;
 def warn_type_attribute_wrong_type : Warning<
diff --git a/clang/include/clang/Sema/ParsedAttr.h b/clang/include/clang/Sema/ParsedAttr.h
index 4fa5fbdb5a7f63..e1faab205f647a 100644
--- a/clang/include/clang/Sema/ParsedAttr.h
+++ b/clang/include/clang/Sema/ParsedAttr.h
@@ -1099,6 +1099,13 @@ enum AttributeDeclKind {
   ExpectedFunctionVariableOrClass,
   ExpectedKernelFunction,
   ExpectedFunctionWithProtoType,
+  ExpectedForLoopStatement,
+  ExpectedVirtualFunction,
+  ExpectedParameterOrImplicitObjectParameter,
+  ExpectedNonMemberFunction,
+  ExpectedFunctionOrClassOrEnum,
+  ExpectedClass,
+  ExpectedTypedef,
 };
 
 inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,
diff --git a/clang/lib/Parse/ParseDecl.cpp b/clang/lib/Parse/ParseDecl.cpp
index 7f3f6d568e28cf..f136d5007e8a5f 100644
--- a/clang/lib/Parse/ParseDecl.cpp
+++ b/clang/lib/Parse/ParseDecl.cpp
@@ -24,6 +24,7 @@
 #include "clang/Parse/RAIIObjectsForParser.h"
 #include "clang/Sema/EnterExpressionEvaluationContext.h"
 #include "clang/Sema/Lookup.h"
+#include "clang/Sema/ParsedAttr.h"
 #include "clang/Sema/ParsedTemplate.h"
 #include "clang/Sema/Scope.h"
 #include "clang/Sema/SemaCUDA.h"
@@ -3708,9 +3709,9 @@ void Parser::ParseDeclarationSpecifiers(
             continue;
 
           if (PA.getKind() == ParsedAttr::AT_LifetimeBound)
-            Diag(PA.getLoc(), diag::err_attribute_wrong_decl_type_str)
+            Diag(PA.getLoc(), diag::err_attribute_wrong_decl_type)
                 << PA << PA.isRegularKeywordAttribute()
-                << "parameters and implicit object parameters";
+                << ExpectedParameterOrImplicitObjectParameter;
           else
             Diag(PA.getLoc(), diag::err_attribute_not_type_attr)
                 << PA << PA.isRegularKeywordAttribute();
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index bb4d33560b93b8..c1663f2d15c88b 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -1868,8 +1868,8 @@ static void handleNakedAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
     // This form is not allowed to be written on a member function (static or
     // nonstatic) when in Microsoft compatibility mode.
     if (S.getLangOpts().MSVCCompat && isa<CXXMethodDecl>(D)) {
-      S.Diag(AL.getLoc(), diag::err_attribute_wrong_decl_type_str)
-          << AL << AL.isRegularKeywordAttribute() << "non-member functions";
+      S.Diag(AL.getLoc(), diag::err_attribute_wrong_decl_type)
+          << AL << AL.isRegularKeywordAttribute() << ExpectedNonMemberFunction;
       return;
     }
   }
@@ -2761,9 +2761,9 @@ static void handleWarnUnusedResult(Sema &S, Decl *D, const ParsedAttr &AL) {
     // The standard attribute cannot be applied to variable declarations such
     // as a function pointer.
     if (isa<VarDecl>(D))
-      S.Diag(AL.getLoc(), diag::warn_attribute_wrong_decl_type_str)
+      S.Diag(AL.getLoc(), diag::warn_attribute_wrong_decl_type)
           << AL << AL.isRegularKeywordAttribute()
-          << "functions, classes, or enumerations";
+          << ExpectedFunctionOrClassOrEnum;
 
     // If this is spelled as the standard C++17 attribute, but not in C++17,
     // warn about using it as an extension. If there are attribute arguments,
@@ -5555,8 +5555,8 @@ static void handleNullableTypeAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
 
   if (auto *CRD = dyn_cast<CXXRecordDecl>(D);
       !CRD || !(CRD->isClass() || CRD->isStruct())) {
-    S.Diag(AL.getRange().getBegin(), diag::err_attribute_wrong_decl_type_str)
-        << AL << AL.isRegularKeywordAttribute() << "classes";
+    S.Diag(AL.getRange().getBegin(), diag::err_attribute_wrong_decl_type)
+        << AL << AL.isRegularKeywordAttribute() << ExpectedClass;
     return;
   }
 
diff --git a/clang/lib/Sema/SemaSwift.cpp b/clang/lib/Sema/SemaSwift.cpp
index 24fdfb8e57dc34..fe72d6c85c37a3 100644
--- a/clang/lib/Sema/SemaSwift.cpp
+++ b/clang/lib/Sema/SemaSwift.cpp
@@ -650,8 +650,8 @@ void SemaSwift::handleNewType(Decl *D, const ParsedAttr &AL) {
   }
 
   if (!isa<TypedefNameDecl>(D)) {
-    Diag(AL.getLoc(), diag::warn_attribute_wrong_decl_type_str)
-        << AL << AL.isRegularKeywordAttribute() << "typedefs";
+    Diag(AL.getLoc(), diag::warn_attribute_wrong_decl_type)
+        << AL << AL.isRegularKeywordAttribute() << ExpectedTypedef;
     return;
   }
 
diff --git a/clang/lib/Sema/SemaType.cpp b/clang/lib/Sema/SemaType.cpp
index d46fbca9ee9768..ba3cddb42c8306 100644
--- a/clang/lib/Sema/SemaType.cpp
+++ b/clang/lib/Sema/SemaType.cpp
@@ -7941,8 +7941,9 @@ static bool handleFunctionTypeAttr(TypeProcessingState &state, ParsedAttr &attr,
     if (!FnTy) {
       // SME ACLE attributes are not supported on K&R-style unprototyped C
       // functions.
-      S.Diag(attr.getLoc(), diag::warn_attribute_wrong_decl_type) <<
-        attr << attr.isRegularKeywordAttribute() << ExpectedFunctionWithProtoType;
+      S.Diag(attr.getLoc(), diag::warn_attribute_wrong_decl_type)
+          << attr << attr.isRegularKeywordAttribute()
+          << ExpectedFunctionWithProtoType;
       attr.setInvalid();
       return false;
     }
@@ -8630,9 +8631,9 @@ static void HandleLifetimeBoundAttr(TypeProcessingState &State,
         CurType, CurType);
     return;
   }
-  State.getSema().Diag(Attr.getLoc(), diag::err_attribute_wrong_decl_type_str)
+  State.getSema().Diag(Attr.getLoc(), diag::err_attribute_wrong_decl_type)
       << Attr << Attr.isRegularKeywordAttribute()
-      << "parameters and implicit object parameters";
+      << ExpectedParameterOrImplicitObjectParameter;
 }
 
 static void HandleLifetimeCaptureByAttr(TypeProcessingState &State,

emaxx-google · 2025-01-10T15:29:00Z

@ilya-biryukov You suggested previously (https://github.com/llvm/llvm-project/pull/118567/files#r1868729303) to introduce a separate enum, but I didn't manage to do it so far. Instead, I added new strings to AttributeDeclKind. My thinking has been:

If we create a enum, I'm not clear on how it'd be different from AttributeDeclKind.
- This AttributeDeclKind is already used both by Parser and Sema - same is for our callsites in question.
- Not sure about how to name the items differently - they do seem very close.
Another idea was to include the old enum's items into the new one.
- But I don't know how, technically, could we do it without copy-pasting the enum items and %select arguments.

But I'm open to any suggestions; thanks!

ilya-biryukov · 2025-01-10T16:47:56Z

@ilya-biryukov You suggested previously (https://github.com/llvm/llvm-project/pull/118567/files#r1868729303) to introduce a separate enum, but I didn't manage to do it so far. Instead, I added new strings to AttributeDeclKind. My thinking has been:

I got confused and thought the AttributeDeclKind is something that's used outside of diagnostics and diagnostics simply reuse it. But that's not the case, so the change LG!

ilya-biryukov

This looks like a clear improvement. Just a quick check on whether we can remove the _str diagnostics completely (see the relevant comment)

clang/lib/Parse/ParseDecl.cpp

ilya-biryukov

LGTM

A cleanup follow-up to llvm#118501 and llvm#118567.

[clang] Refactor attr diagnostics to use %select

67c2e43

emaxx-google marked this pull request as ready for review January 10, 2025 15:21

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jan 10, 2025

ilya-biryukov reviewed Jan 10, 2025

View reviewed changes

clang/lib/Parse/ParseDecl.cpp Show resolved Hide resolved

This was referenced Jan 10, 2025

(reland) [clang] Warn [[clang::lifetimebound]] misusages on types #118501

Merged

[clang] Informative error for lifetimebound in decl-spec #118567

Merged

emaxx-google requested a review from ilya-biryukov January 10, 2025 19:43

ilya-biryukov approved these changes Jan 13, 2025

View reviewed changes

ilya-biryukov merged commit 41a94de into llvm:main Jan 13, 2025
13 checks passed

kazutakahirata pushed a commit to kazutakahirata/llvm-project that referenced this pull request Jan 13, 2025

[clang] Refactor attr diagnostics to use %select (llvm#122473)

9a9b71c

A cleanup follow-up to llvm#118501 and llvm#118567.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[clang] Refactor attr diagnostics to use %select #122473

[clang] Refactor attr diagnostics to use %select #122473

Uh oh!

emaxx-google commented Jan 10, 2025 •

edited

Loading

Uh oh!

llvmbot commented Jan 10, 2025

Uh oh!

emaxx-google commented Jan 10, 2025 •

edited

Loading

Uh oh!

ilya-biryukov commented Jan 10, 2025

Uh oh!

ilya-biryukov left a comment

Uh oh!

Uh oh!

ilya-biryukov left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[clang] Refactor attr diagnostics to use %select #122473

[clang] Refactor attr diagnostics to use %select #122473

Uh oh!

Conversation

emaxx-google commented Jan 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jan 10, 2025

Uh oh!

emaxx-google commented Jan 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ilya-biryukov commented Jan 10, 2025

Uh oh!

ilya-biryukov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ilya-biryukov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

emaxx-google commented Jan 10, 2025 •

edited

Loading

emaxx-google commented Jan 10, 2025 •

edited

Loading