[clang] Fix the double space and double attribute printing of the final keyword. (PR #88600)
kimgr wrote: @vgvassilev Thank you! https://github.com/llvm/llvm-project/pull/88600 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix the double space and double attribute printing of the final keyword. (PR #88600)
https://github.com/vgvassilev closed https://github.com/llvm/llvm-project/pull/88600 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix the double space and double attribute printing of the final keyword. (PR #88600)
https://github.com/vgvassilev updated
https://github.com/llvm/llvm-project/pull/88600
>From 9b2bb9068cbefcfffd0931fbbee46b1a0f536a4f Mon Sep 17 00:00:00 2001
From: Vassil Vassilev
Date: Sat, 13 Apr 2024 06:39:34 +
Subject: [PATCH 1/2] Fix the double space and double attribute printing of the
final keyword.
Fixes #56517.
---
clang/lib/AST/DeclPrinter.cpp | 36 +++-
clang/test/SemaCXX/cxx11-attr-print.cpp | 5 +++
clang/test/SemaHLSL/BuiltIns/RWBuffers.hlsl | 4 +--
clang/unittests/AST/DeclPrinterTest.cpp | 37 -
clang/utils/TableGen/ClangAttrEmitter.cpp | 2 +-
5 files changed, 57 insertions(+), 27 deletions(-)
diff --git a/clang/lib/AST/DeclPrinter.cpp b/clang/lib/AST/DeclPrinter.cpp
index 6afdb6cfccb142..444780947f2075 100644
--- a/clang/lib/AST/DeclPrinter.cpp
+++ b/clang/lib/AST/DeclPrinter.cpp
@@ -119,7 +119,7 @@ namespace {
void printTemplateArguments(llvm::ArrayRef Args,
const TemplateParameterList *Params);
enum class AttrPosAsWritten { Default = 0, Left, Right };
-void
+bool
prettyPrintAttributes(const Decl *D,
AttrPosAsWritten Pos = AttrPosAsWritten::Default);
void prettyPrintPragmas(Decl *D);
@@ -252,16 +252,19 @@ static DeclPrinter::AttrPosAsWritten
getPosAsWritten(const Attr *A,
return DeclPrinter::AttrPosAsWritten::Right;
}
-void DeclPrinter::prettyPrintAttributes(const Decl *D,
+// returns true if an attribute was printed.
+bool DeclPrinter::prettyPrintAttributes(const Decl *D,
AttrPosAsWritten Pos /*=Default*/) {
- if (Policy.PolishForDeclaration)
-return;
+ bool hasPrinted = false;
if (D->hasAttrs()) {
const AttrVec = D->getAttrs();
for (auto *A : Attrs) {
if (A->isInherited() || A->isImplicit())
continue;
+ // Don't strip out the keyword attributes, they aren't regular
attributes.
+ if (Policy.PolishForDeclaration && !A->isKeywordAttribute())
+continue;
switch (A->getKind()) {
#define ATTR(X)
#define PRAGMA_SPELLING_ATTR(X) case attr::X:
@@ -275,6 +278,7 @@ void DeclPrinter::prettyPrintAttributes(const Decl *D,
if (Pos != AttrPosAsWritten::Left)
Out << ' ';
A->printPretty(Out, Policy);
+ hasPrinted = true;
if (Pos == AttrPosAsWritten::Left)
Out << ' ';
}
@@ -282,6 +286,7 @@ void DeclPrinter::prettyPrintAttributes(const Decl *D,
}
}
}
+ return hasPrinted;
}
void DeclPrinter::prettyPrintPragmas(Decl *D) {
@@ -1060,12 +1065,15 @@ void DeclPrinter::VisitCXXRecordDecl(CXXRecordDecl *D) {
// FIXME: add printing of pragma attributes if required.
if (!Policy.SuppressSpecifiers && D->isModulePrivate())
Out << "__module_private__ ";
- Out << D->getKindName();
- prettyPrintAttributes(D);
+ Out << D->getKindName() << ' ';
- if (D->getIdentifier()) {
+ // FIXME: Move before printing the decl kind to match the behavior of the
+ // attribute printing for variables and function where they are printed
first.
+ if (prettyPrintAttributes(D, AttrPosAsWritten::Left))
Out << ' ';
+
+ if (D->getIdentifier()) {
if (auto *NNS = D->getQualifier())
NNS->print(Out, Policy);
Out << *D;
@@ -1082,16 +1090,13 @@ void DeclPrinter::VisitCXXRecordDecl(CXXRecordDecl *D) {
}
}
- if (D->hasDefinition()) {
-if (D->hasAttr()) {
- Out << " final";
-}
- }
+ prettyPrintAttributes(D, AttrPosAsWritten::Right);
if (D->isCompleteDefinition()) {
+Out << ' ';
// Print the base classes
if (D->getNumBases()) {
- Out << " : ";
+ Out << ": ";
for (CXXRecordDecl::base_class_iterator Base = D->bases_begin(),
BaseEnd = D->bases_end(); Base != BaseEnd; ++Base) {
if (Base != D->bases_begin())
@@ -1110,14 +1115,15 @@ void DeclPrinter::VisitCXXRecordDecl(CXXRecordDecl *D) {
if (Base->isPackExpansion())
Out << "...";
}
+ Out << ' ';
}
// Print the class definition
// FIXME: Doesn't print access specifiers, e.g., "public:"
if (Policy.TerseOutput) {
- Out << " {}";
+ Out << "{}";
} else {
- Out << " {\n";
+ Out << "{\n";
VisitDeclContext(D);
Indent() << "}";
}
diff --git a/clang/test/SemaCXX/cxx11-attr-print.cpp
b/clang/test/SemaCXX/cxx11-attr-print.cpp
index a169d1b4409b4d..2b084018bc0662 100644
--- a/clang/test/SemaCXX/cxx11-attr-print.cpp
+++ b/clang/test/SemaCXX/cxx11-attr-print.cpp
@@ -87,3 +87,8 @@ template struct S;
// CHECK: using Small2 {{\[}}[gnu::mode(byte)]] = int;
using Small2 [[gnu::mode(byte)]] = int;
+
+class FinalNonTemplate final {};
+// CHECK: class FinalNonTemplate final {
+template class FinalTemplate final {};
+// CHECK: template class FinalTemplate final {
diff --git a/clang/test/SemaHLSL/BuiltIns/RWBuffers.hlsl
[clang] Fix the double space and double attribute printing of the final keyword. (PR #88600)
@@ -275,13 +278,15 @@ void DeclPrinter::prettyPrintAttributes(const Decl *D, if (Pos != AttrPosAsWritten::Left) Out << ' '; A->printPretty(Out, Policy); + hasPrinted = true; if (Pos == AttrPosAsWritten::Left) Out << ' '; } break; } } } + return hasPrinted; vgvassilev wrote: I was thinking to use something like that but I believe the boolean flag is more readable. I'd like to keep it the way it is. https://github.com/llvm/llvm-project/pull/88600 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix the double space and double attribute printing of the final keyword. (PR #88600)
@@ -252,16 +252,19 @@ static DeclPrinter::AttrPosAsWritten
getPosAsWritten(const Attr *A,
return DeclPrinter::AttrPosAsWritten::Right;
}
-void DeclPrinter::prettyPrintAttributes(const Decl *D,
+// returns true if an attribute was printed.
+bool DeclPrinter::prettyPrintAttributes(const Decl *D,
AttrPosAsWritten Pos /*=Default*/) {
- if (Policy.PolishForDeclaration)
-return;
+ bool hasPrinted = false;
if (D->hasAttrs()) {
const AttrVec = D->getAttrs();
for (auto *A : Attrs) {
if (A->isInherited() || A->isImplicit())
continue;
+ // Don't strip out the keyword attributes, they aren't regular
attributes.
vgvassilev wrote:
We should word it as `Print out the keyword attributes, they aren't regular
attributes.`, I think.
https://github.com/llvm/llvm-project/pull/88600
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix the double space and double attribute printing of the final keyword. (PR #88600)
@@ -252,16 +252,19 @@ static DeclPrinter::AttrPosAsWritten
getPosAsWritten(const Attr *A,
return DeclPrinter::AttrPosAsWritten::Right;
}
-void DeclPrinter::prettyPrintAttributes(const Decl *D,
+// returns true if an attribute was printed.
+bool DeclPrinter::prettyPrintAttributes(const Decl *D,
AttrPosAsWritten Pos /*=Default*/) {
- if (Policy.PolishForDeclaration)
-return;
+ bool hasPrinted = false;
if (D->hasAttrs()) {
const AttrVec = D->getAttrs();
for (auto *A : Attrs) {
if (A->isInherited() || A->isImplicit())
continue;
+ // Don't strip out the keyword attributes, they aren't regular
attributes.
erichkeane wrote:
```suggestion
// Don't print out the keyword attributes, they aren't regular attributes.
```
??
https://github.com/llvm/llvm-project/pull/88600
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix the double space and double attribute printing of the final keyword. (PR #88600)
@@ -275,13 +278,15 @@ void DeclPrinter::prettyPrintAttributes(const Decl *D, if (Pos != AttrPosAsWritten::Left) Out << ' '; A->printPretty(Out, Policy); + hasPrinted = true; if (Pos == AttrPosAsWritten::Left) Out << ' '; } break; } } } + return hasPrinted; erichkeane wrote: see `Out->tell()` here, it is perhaps less error-prone here (that is, store the `uint64_t` value above, and see if it changed). https://github.com/llvm/llvm-project/pull/88600 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix the double space and double attribute printing of the final keyword. (PR #88600)
https://github.com/erichkeane approved this pull request. 2 nits, else LGTM. Once you and @kimgr are in agreement with what to do next (and have looked into my nits), feel free to commit. https://github.com/llvm/llvm-project/pull/88600 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix the double space and double attribute printing of the final keyword. (PR #88600)
https://github.com/erichkeane edited https://github.com/llvm/llvm-project/pull/88600 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix the double space and double attribute printing of the final keyword. (PR #88600)
kimgr wrote: > I am wondering if that'd be interesting and if so, maybe we can share it > between both projects via upstreaming to Clang... That sounds fantastic, but mostly because I don't have anything to offer, and everything to benefit :) I was just thinking about adding a `.ForwardDeclaration` policy to `DeclPrinter` this morning -- the current `PolishForDeclaration` output is _so_ close. But it seemed weird to add a mode for which IWYU (a non-LLVM project) was the only user. I suspect your `ForwardDeclarePrinter` is more principled. I guess it might be hard to coordinate forward-decl style between users, but ultimately that's just printing policy (or chaining libFormat, or something). Let's maybe continue this discussion somewhere else? https://github.com/llvm/llvm-project/pull/88600 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix the double space and double attribute printing of the final keyword. (PR #88600)
kimgr wrote: Current PR passes all my tests, both Clang (`ninja ASTTests`), Clangd (`ninja ClangdTests`) and IWYU end-to-end tests -- thanks! https://github.com/llvm/llvm-project/pull/88600 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix the double space and double attribute printing of the final keyword. (PR #88600)
vgvassilev wrote: > * IWYU uses `PolishForDeclaration` to get a valid _declaration_, and then > does [some very hacky heuristic > post-processing](https://github.com/include-what-you-use/include-what-you-use/blob/125341c412ceee9233ece8973848b49e770a9b82/iwyu_output.cc#L469) > to turn it into a forward-decl. The [ROOT](http://root.cern) usecase of the Cling interpreter has similar infrastructure and it works at scale couple of million lines of scientific codes: https://github.com/root-project/root/blob/master/interpreter/cling/lib/Interpreter/ForwardDeclPrinter.cpp. There are some tests that one can look at: https://github.com/root-project/root/tree/master/interpreter/cling/test/Autoloading I am wondering if that'd be interesting and if so, maybe we can share it between both projects via upstreaming to Clang... https://github.com/llvm/llvm-project/pull/88600 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix the double space and double attribute printing of the final keyword. (PR #88600)
@@ -252,16 +252,19 @@ static DeclPrinter::AttrPosAsWritten
getPosAsWritten(const Attr *A,
return DeclPrinter::AttrPosAsWritten::Right;
}
-void DeclPrinter::prettyPrintAttributes(const Decl *D,
+// returns true if an attribute was printed.
+bool DeclPrinter::prettyPrintAttributes(const Decl *D,
AttrPosAsWritten Pos /*=Default*/) {
- if (Policy.PolishForDeclaration)
-return;
+ bool hasPrinted = false;
if (D->hasAttrs()) {
const AttrVec = D->getAttrs();
for (auto *A : Attrs) {
if (A->isInherited() || A->isImplicit())
continue;
+ // Don't strip out the keyword attributes, they aren't regular
attributes.
+ if (Policy.PolishForDeclaration && !A->isKeywordAttribute())
kimgr wrote:
Nice, I was about to suggest something like this, but didn't know
`isKeywordAttribute` existed.
https://github.com/llvm/llvm-project/pull/88600
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix the double space and double attribute printing of the final keyword. (PR #88600)
kimgr wrote: > If the intent is to produce a forward declaration the final keyword cannot be > attached to a forward declaration. So I am not sure what's the "right" fix > here... I don't believe that's the intent of `DeclPrinter` or `PolishForDeclaration` -- * Clangd uses `PolishForDeclaration` to print an informational "hover text" for the declaration (and I guess that's why they want to include `final` -- it's something that's good for an interactive user to know about the decl) * IWYU uses `PolishForDeclaration` to get a valid _declaration_, and then does [some very hacky heuristic post-processing](https://github.com/include-what-you-use/include-what-you-use/blob/125341c412ceee9233ece8973848b49e770a9b82/iwyu_output.cc#L469) to turn it into a forward-decl. Sorry for stirring confusion into this, my primary focus is IWYU, but I remember that the original double-final came from a change intended for Clangd. Hopefully this helps clarify. https://github.com/llvm/llvm-project/pull/88600 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix the double space and double attribute printing of the final keyword. (PR #88600)
https://github.com/vgvassilev updated
https://github.com/llvm/llvm-project/pull/88600
>From 9b2bb9068cbefcfffd0931fbbee46b1a0f536a4f Mon Sep 17 00:00:00 2001
From: Vassil Vassilev
Date: Sat, 13 Apr 2024 06:39:34 +
Subject: [PATCH] Fix the double space and double attribute printing of the
final keyword.
Fixes #56517.
---
clang/lib/AST/DeclPrinter.cpp | 36 +++-
clang/test/SemaCXX/cxx11-attr-print.cpp | 5 +++
clang/test/SemaHLSL/BuiltIns/RWBuffers.hlsl | 4 +--
clang/unittests/AST/DeclPrinterTest.cpp | 37 -
clang/utils/TableGen/ClangAttrEmitter.cpp | 2 +-
5 files changed, 57 insertions(+), 27 deletions(-)
diff --git a/clang/lib/AST/DeclPrinter.cpp b/clang/lib/AST/DeclPrinter.cpp
index 6afdb6cfccb142..444780947f2075 100644
--- a/clang/lib/AST/DeclPrinter.cpp
+++ b/clang/lib/AST/DeclPrinter.cpp
@@ -119,7 +119,7 @@ namespace {
void printTemplateArguments(llvm::ArrayRef Args,
const TemplateParameterList *Params);
enum class AttrPosAsWritten { Default = 0, Left, Right };
-void
+bool
prettyPrintAttributes(const Decl *D,
AttrPosAsWritten Pos = AttrPosAsWritten::Default);
void prettyPrintPragmas(Decl *D);
@@ -252,16 +252,19 @@ static DeclPrinter::AttrPosAsWritten
getPosAsWritten(const Attr *A,
return DeclPrinter::AttrPosAsWritten::Right;
}
-void DeclPrinter::prettyPrintAttributes(const Decl *D,
+// returns true if an attribute was printed.
+bool DeclPrinter::prettyPrintAttributes(const Decl *D,
AttrPosAsWritten Pos /*=Default*/) {
- if (Policy.PolishForDeclaration)
-return;
+ bool hasPrinted = false;
if (D->hasAttrs()) {
const AttrVec = D->getAttrs();
for (auto *A : Attrs) {
if (A->isInherited() || A->isImplicit())
continue;
+ // Don't strip out the keyword attributes, they aren't regular
attributes.
+ if (Policy.PolishForDeclaration && !A->isKeywordAttribute())
+continue;
switch (A->getKind()) {
#define ATTR(X)
#define PRAGMA_SPELLING_ATTR(X) case attr::X:
@@ -275,6 +278,7 @@ void DeclPrinter::prettyPrintAttributes(const Decl *D,
if (Pos != AttrPosAsWritten::Left)
Out << ' ';
A->printPretty(Out, Policy);
+ hasPrinted = true;
if (Pos == AttrPosAsWritten::Left)
Out << ' ';
}
@@ -282,6 +286,7 @@ void DeclPrinter::prettyPrintAttributes(const Decl *D,
}
}
}
+ return hasPrinted;
}
void DeclPrinter::prettyPrintPragmas(Decl *D) {
@@ -1060,12 +1065,15 @@ void DeclPrinter::VisitCXXRecordDecl(CXXRecordDecl *D) {
// FIXME: add printing of pragma attributes if required.
if (!Policy.SuppressSpecifiers && D->isModulePrivate())
Out << "__module_private__ ";
- Out << D->getKindName();
- prettyPrintAttributes(D);
+ Out << D->getKindName() << ' ';
- if (D->getIdentifier()) {
+ // FIXME: Move before printing the decl kind to match the behavior of the
+ // attribute printing for variables and function where they are printed
first.
+ if (prettyPrintAttributes(D, AttrPosAsWritten::Left))
Out << ' ';
+
+ if (D->getIdentifier()) {
if (auto *NNS = D->getQualifier())
NNS->print(Out, Policy);
Out << *D;
@@ -1082,16 +1090,13 @@ void DeclPrinter::VisitCXXRecordDecl(CXXRecordDecl *D) {
}
}
- if (D->hasDefinition()) {
-if (D->hasAttr()) {
- Out << " final";
-}
- }
+ prettyPrintAttributes(D, AttrPosAsWritten::Right);
if (D->isCompleteDefinition()) {
+Out << ' ';
// Print the base classes
if (D->getNumBases()) {
- Out << " : ";
+ Out << ": ";
for (CXXRecordDecl::base_class_iterator Base = D->bases_begin(),
BaseEnd = D->bases_end(); Base != BaseEnd; ++Base) {
if (Base != D->bases_begin())
@@ -1110,14 +1115,15 @@ void DeclPrinter::VisitCXXRecordDecl(CXXRecordDecl *D) {
if (Base->isPackExpansion())
Out << "...";
}
+ Out << ' ';
}
// Print the class definition
// FIXME: Doesn't print access specifiers, e.g., "public:"
if (Policy.TerseOutput) {
- Out << " {}";
+ Out << "{}";
} else {
- Out << " {\n";
+ Out << "{\n";
VisitDeclContext(D);
Indent() << "}";
}
diff --git a/clang/test/SemaCXX/cxx11-attr-print.cpp
b/clang/test/SemaCXX/cxx11-attr-print.cpp
index a169d1b4409b4d..2b084018bc0662 100644
--- a/clang/test/SemaCXX/cxx11-attr-print.cpp
+++ b/clang/test/SemaCXX/cxx11-attr-print.cpp
@@ -87,3 +87,8 @@ template struct S;
// CHECK: using Small2 {{\[}}[gnu::mode(byte)]] = int;
using Small2 [[gnu::mode(byte)]] = int;
+
+class FinalNonTemplate final {};
+// CHECK: class FinalNonTemplate final {
+template class FinalTemplate final {};
+// CHECK: template class FinalTemplate final {
diff --git a/clang/test/SemaHLSL/BuiltIns/RWBuffers.hlsl
[clang] Fix the double space and double attribute printing of the final keyword. (PR #88600)
https://github.com/vgvassilev updated
https://github.com/llvm/llvm-project/pull/88600
>From cb3da95dd80c5ed991c5342e655e0f170eab16eb Mon Sep 17 00:00:00 2001
From: Vassil Vassilev
Date: Sat, 13 Apr 2024 06:39:34 +
Subject: [PATCH] Fix the double space and double attribute printing of the
final keyword.
Fixes #56517.
---
clang/lib/AST/DeclPrinter.cpp | 36 +++-
clang/test/SemaCXX/cxx11-attr-print.cpp | 5 +++
clang/test/SemaHLSL/BuiltIns/RWBuffers.hlsl | 4 +--
clang/unittests/AST/DeclPrinterTest.cpp | 37 -
clang/utils/TableGen/ClangAttrEmitter.cpp | 2 +-
5 files changed, 57 insertions(+), 27 deletions(-)
diff --git a/clang/lib/AST/DeclPrinter.cpp b/clang/lib/AST/DeclPrinter.cpp
index 6afdb6cfccb142..f5ae5d5b36449a 100644
--- a/clang/lib/AST/DeclPrinter.cpp
+++ b/clang/lib/AST/DeclPrinter.cpp
@@ -119,7 +119,7 @@ namespace {
void printTemplateArguments(llvm::ArrayRef Args,
const TemplateParameterList *Params);
enum class AttrPosAsWritten { Default = 0, Left, Right };
-void
+bool
prettyPrintAttributes(const Decl *D,
AttrPosAsWritten Pos = AttrPosAsWritten::Default);
void prettyPrintPragmas(Decl *D);
@@ -252,16 +252,19 @@ static DeclPrinter::AttrPosAsWritten
getPosAsWritten(const Attr *A,
return DeclPrinter::AttrPosAsWritten::Right;
}
-void DeclPrinter::prettyPrintAttributes(const Decl *D,
+// returns true if an attribute was printed.
+bool DeclPrinter::prettyPrintAttributes(const Decl *D,
AttrPosAsWritten Pos /*=Default*/) {
- if (Policy.PolishForDeclaration)
-return;
+ bool hasPrinted = false;
if (D->hasAttrs()) {
const AttrVec = D->getAttrs();
for (auto *A : Attrs) {
if (A->isInherited() || A->isImplicit())
continue;
+ // Don't strip out the keyword attributes, they aren't regular
attributes.
+ if (Policy.PolishForDeclaration && A->isKeywordAttribute())
+continue;
switch (A->getKind()) {
#define ATTR(X)
#define PRAGMA_SPELLING_ATTR(X) case attr::X:
@@ -275,6 +278,7 @@ void DeclPrinter::prettyPrintAttributes(const Decl *D,
if (Pos != AttrPosAsWritten::Left)
Out << ' ';
A->printPretty(Out, Policy);
+ hasPrinted = true;
if (Pos == AttrPosAsWritten::Left)
Out << ' ';
}
@@ -282,6 +286,7 @@ void DeclPrinter::prettyPrintAttributes(const Decl *D,
}
}
}
+ return hasPrinted;
}
void DeclPrinter::prettyPrintPragmas(Decl *D) {
@@ -1060,12 +1065,15 @@ void DeclPrinter::VisitCXXRecordDecl(CXXRecordDecl *D) {
// FIXME: add printing of pragma attributes if required.
if (!Policy.SuppressSpecifiers && D->isModulePrivate())
Out << "__module_private__ ";
- Out << D->getKindName();
- prettyPrintAttributes(D);
+ Out << D->getKindName() << ' ';
- if (D->getIdentifier()) {
+ // FIXME: Move before printing the decl kind to match the behavior of the
+ // attribute printing for variables and function where they are printed
first.
+ if (prettyPrintAttributes(D, AttrPosAsWritten::Left))
Out << ' ';
+
+ if (D->getIdentifier()) {
if (auto *NNS = D->getQualifier())
NNS->print(Out, Policy);
Out << *D;
@@ -1082,16 +1090,13 @@ void DeclPrinter::VisitCXXRecordDecl(CXXRecordDecl *D) {
}
}
- if (D->hasDefinition()) {
-if (D->hasAttr()) {
- Out << " final";
-}
- }
+ prettyPrintAttributes(D, AttrPosAsWritten::Right);
if (D->isCompleteDefinition()) {
+Out << ' ';
// Print the base classes
if (D->getNumBases()) {
- Out << " : ";
+ Out << ": ";
for (CXXRecordDecl::base_class_iterator Base = D->bases_begin(),
BaseEnd = D->bases_end(); Base != BaseEnd; ++Base) {
if (Base != D->bases_begin())
@@ -1110,14 +1115,15 @@ void DeclPrinter::VisitCXXRecordDecl(CXXRecordDecl *D) {
if (Base->isPackExpansion())
Out << "...";
}
+ Out << ' ';
}
// Print the class definition
// FIXME: Doesn't print access specifiers, e.g., "public:"
if (Policy.TerseOutput) {
- Out << " {}";
+ Out << "{}";
} else {
- Out << " {\n";
+ Out << "{\n";
VisitDeclContext(D);
Indent() << "}";
}
diff --git a/clang/test/SemaCXX/cxx11-attr-print.cpp
b/clang/test/SemaCXX/cxx11-attr-print.cpp
index a169d1b4409b4d..2b084018bc0662 100644
--- a/clang/test/SemaCXX/cxx11-attr-print.cpp
+++ b/clang/test/SemaCXX/cxx11-attr-print.cpp
@@ -87,3 +87,8 @@ template struct S;
// CHECK: using Small2 {{\[}}[gnu::mode(byte)]] = int;
using Small2 [[gnu::mode(byte)]] = int;
+
+class FinalNonTemplate final {};
+// CHECK: class FinalNonTemplate final {
+template class FinalTemplate final {};
+// CHECK: template class FinalTemplate final {
diff --git a/clang/test/SemaHLSL/BuiltIns/RWBuffers.hlsl
[clang] Fix the double space and double attribute printing of the final keyword. (PR #88600)
vgvassilev wrote: I've put a fix that fixes the cases that you mentioned... https://github.com/llvm/llvm-project/pull/88600 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix the double space and double attribute printing of the final keyword. (PR #88600)
https://github.com/vgvassilev updated
https://github.com/llvm/llvm-project/pull/88600
>From c68344d2d2f22f88ef386f655cc7698759fb551c Mon Sep 17 00:00:00 2001
From: Vassil Vassilev
Date: Sat, 13 Apr 2024 06:39:34 +
Subject: [PATCH] Fix the double space and double attribute printing of the
final keyword.
Fixes #56517.
---
clang/lib/AST/DeclPrinter.cpp | 36 +++-
clang/test/SemaCXX/cxx11-attr-print.cpp | 5 +++
clang/test/SemaHLSL/BuiltIns/RWBuffers.hlsl | 4 +--
clang/unittests/AST/DeclPrinterTest.cpp | 37 -
clang/utils/TableGen/ClangAttrEmitter.cpp | 2 +-
5 files changed, 57 insertions(+), 27 deletions(-)
diff --git a/clang/lib/AST/DeclPrinter.cpp b/clang/lib/AST/DeclPrinter.cpp
index 6afdb6cfccb142..5d15541a6d54c5 100644
--- a/clang/lib/AST/DeclPrinter.cpp
+++ b/clang/lib/AST/DeclPrinter.cpp
@@ -119,7 +119,7 @@ namespace {
void printTemplateArguments(llvm::ArrayRef Args,
const TemplateParameterList *Params);
enum class AttrPosAsWritten { Default = 0, Left, Right };
-void
+bool
prettyPrintAttributes(const Decl *D,
AttrPosAsWritten Pos = AttrPosAsWritten::Default);
void prettyPrintPragmas(Decl *D);
@@ -252,16 +252,19 @@ static DeclPrinter::AttrPosAsWritten
getPosAsWritten(const Attr *A,
return DeclPrinter::AttrPosAsWritten::Right;
}
-void DeclPrinter::prettyPrintAttributes(const Decl *D,
+// returns true if an attribute was printed.
+bool DeclPrinter::prettyPrintAttributes(const Decl *D,
AttrPosAsWritten Pos /*=Default*/) {
- if (Policy.PolishForDeclaration)
-return;
+ bool hasPrinted = false;
if (D->hasAttrs()) {
const AttrVec = D->getAttrs();
for (auto *A : Attrs) {
if (A->isInherited() || A->isImplicit())
continue;
+ if (Policy.PolishForDeclaration &&
+ A->getSyntax() != AttributeCommonInfo::AS_Keyword)
+continue;
switch (A->getKind()) {
#define ATTR(X)
#define PRAGMA_SPELLING_ATTR(X) case attr::X:
@@ -275,6 +278,7 @@ void DeclPrinter::prettyPrintAttributes(const Decl *D,
if (Pos != AttrPosAsWritten::Left)
Out << ' ';
A->printPretty(Out, Policy);
+ hasPrinted = true;
if (Pos == AttrPosAsWritten::Left)
Out << ' ';
}
@@ -282,6 +286,7 @@ void DeclPrinter::prettyPrintAttributes(const Decl *D,
}
}
}
+ return hasPrinted;
}
void DeclPrinter::prettyPrintPragmas(Decl *D) {
@@ -1060,12 +1065,15 @@ void DeclPrinter::VisitCXXRecordDecl(CXXRecordDecl *D) {
// FIXME: add printing of pragma attributes if required.
if (!Policy.SuppressSpecifiers && D->isModulePrivate())
Out << "__module_private__ ";
- Out << D->getKindName();
- prettyPrintAttributes(D);
+ Out << D->getKindName() << ' ';
- if (D->getIdentifier()) {
+ // FIXME: Move before printing the decl kind to match the behavior of the
+ // attribute printing for variables and function where they are printed
first.
+ if (prettyPrintAttributes(D, AttrPosAsWritten::Left))
Out << ' ';
+
+ if (D->getIdentifier()) {
if (auto *NNS = D->getQualifier())
NNS->print(Out, Policy);
Out << *D;
@@ -1082,16 +1090,13 @@ void DeclPrinter::VisitCXXRecordDecl(CXXRecordDecl *D) {
}
}
- if (D->hasDefinition()) {
-if (D->hasAttr()) {
- Out << " final";
-}
- }
+ prettyPrintAttributes(D, AttrPosAsWritten::Right);
if (D->isCompleteDefinition()) {
+Out << ' ';
// Print the base classes
if (D->getNumBases()) {
- Out << " : ";
+ Out << ": ";
for (CXXRecordDecl::base_class_iterator Base = D->bases_begin(),
BaseEnd = D->bases_end(); Base != BaseEnd; ++Base) {
if (Base != D->bases_begin())
@@ -1110,14 +1115,15 @@ void DeclPrinter::VisitCXXRecordDecl(CXXRecordDecl *D) {
if (Base->isPackExpansion())
Out << "...";
}
+ Out << ' ';
}
// Print the class definition
// FIXME: Doesn't print access specifiers, e.g., "public:"
if (Policy.TerseOutput) {
- Out << " {}";
+ Out << "{}";
} else {
- Out << " {\n";
+ Out << "{\n";
VisitDeclContext(D);
Indent() << "}";
}
diff --git a/clang/test/SemaCXX/cxx11-attr-print.cpp
b/clang/test/SemaCXX/cxx11-attr-print.cpp
index a169d1b4409b4d..2b084018bc0662 100644
--- a/clang/test/SemaCXX/cxx11-attr-print.cpp
+++ b/clang/test/SemaCXX/cxx11-attr-print.cpp
@@ -87,3 +87,8 @@ template struct S;
// CHECK: using Small2 {{\[}}[gnu::mode(byte)]] = int;
using Small2 [[gnu::mode(byte)]] = int;
+
+class FinalNonTemplate final {};
+// CHECK: class FinalNonTemplate final {
+template class FinalTemplate final {};
+// CHECK: template class FinalTemplate final {
diff --git a/clang/test/SemaHLSL/BuiltIns/RWBuffers.hlsl
b/clang/test/SemaHLSL/BuiltIns/RWBuffers.hlsl
index
[clang] Fix the double space and double attribute printing of the final keyword. (PR #88600)
vgvassilev wrote: IIUC, the PolishForDeclaration option is supposed to `When true, do certain refinement needed for producing proper declaration tag; such as, do not print attributes attached to the declaration. `. If the intent is to produce a forward declaration the `final` keyword cannot be attached to a forward declaration. So I am not sure what's the "right" fix here... https://github.com/llvm/llvm-project/pull/88600 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix the double space and double attribute printing of the final keyword. (PR #88600)
kimgr wrote:
Thank you.
I believe this should cover both cases, added some attempt at rationale in
comments:
```diff
diff --git a/clang/unittests/AST/DeclPrinterTest.cpp
b/clang/unittests/AST/DeclPrinterTest.cpp
index f2b027a25621..8691a6c38f16 100644
--- a/clang/unittests/AST/DeclPrinterTest.cpp
+++ b/clang/unittests/AST/DeclPrinterTest.cpp
@@ -86,16 +86,13 @@ PrintedDeclCXX98Matches(StringRef Code, const
DeclarationMatcher ,
ExpectedPrinted, "input.cc");
}
-::testing::AssertionResult PrintedDeclCXX11Matches(
- StringRef Code,
- const DeclarationMatcher ,
- StringRef ExpectedPrinted) {
+::testing::AssertionResult
+PrintedDeclCXX11Matches(StringRef Code, const DeclarationMatcher ,
+StringRef ExpectedPrinted,
+PrintingPolicyAdjuster PolicyModifier = nullptr) {
std::vector Args(1, "-std=c++11");
- return PrintedDeclMatches(Code,
-Args,
-NodeMatch,
-ExpectedPrinted,
-"input.cc");
+ return PrintedDeclMatches(Code, Args, NodeMatch, ExpectedPrinted, "input.cc",
+PolicyModifier);
}
::testing::AssertionResult PrintedDeclCXX11nonMSCMatches(
@@ -1556,3 +1553,25 @@ TEST(DeclPrinter, VarDeclWithInitializer) {
PrintedDeclCXX17Matches("void foo() {int arr[42]; for(int a : arr);}",
namedDecl(hasName("a")).bind("id"), "int a"));
}
+
+TEST(DeclPrinter, TestTemplateFinal) {
+ // By default we should print 'final' keyword whether class is implicitly or
+ // explicitly marked final.
+ ASSERT_TRUE(PrintedDeclCXX11Matches(
+ "template\n"
+ "class FinalTemplate final {};",
+ classTemplateDecl(hasName("FinalTemplate")).bind("id"),
+ "template class FinalTemplate final {}"));
+}
+
+TEST(DeclPrinter, TestTemplateFinalWithPolishForDecl) {
+ // clangd relies on the 'final' keyword being printed when
+ // PolishForDeclaration is enabled, so make sure it is even if implicit attrs
+ // are disabled.
+ ASSERT_TRUE(PrintedDeclCXX11Matches(
+ "template\n"
+ "class FinalTemplate final {};",
+ classTemplateDecl(hasName("FinalTemplate")).bind("id"),
+ "template class FinalTemplate final {}",
+ [](PrintingPolicy ) { Policy.PolishForDeclaration = true; }));
+}
```
https://github.com/llvm/llvm-project/pull/88600
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix the double space and double attribute printing of the final keyword. (PR #88600)
vgvassilev wrote: > > With .PolishForDeclaration=true, there are NO final specifiers (which is > > what we want to produce forward decls in IWYU) > > This is actually a regression in this PR, and it breaks the clangd test added > here: > [9f57b65](https://github.com/llvm/llvm-project/commit/9f57b65a272817752aa00e2fb94154e6eed1d0ec) > (the patch that originally led to double `final`s). > > EDIT: It looks like `prettyPrintAttributes` is only intended for semantic > attributes, whereas clangd wanted to preserve as-written `final` keyword. So > I wonder if this change needs to be tweaked a little: > https://github.com/llvm/llvm-project/pull/88600/files#diff-81d69bc555945d6582a758e0c094ff870cbc38697501ad7415694ee30c567dbfL1085 I've added a fix for your example. Can you provide a test case for the clangd use-case? https://github.com/llvm/llvm-project/pull/88600 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix the double space and double attribute printing of the final keyword. (PR #88600)
https://github.com/vgvassilev updated
https://github.com/llvm/llvm-project/pull/88600
>From bcd04db735a78b4d7df93e88229ea4e2491fc09e Mon Sep 17 00:00:00 2001
From: Vassil Vassilev
Date: Sat, 13 Apr 2024 06:39:34 +
Subject: [PATCH] Fix the double space and double attribute printing of the
final keyword.
Fixes #56517.
---
clang/lib/AST/DeclPrinter.cpp | 31 -
clang/test/SemaCXX/cxx11-attr-print.cpp | 5
clang/test/SemaHLSL/BuiltIns/RWBuffers.hlsl | 4 +--
clang/unittests/AST/DeclPrinterTest.cpp | 8 ++
clang/utils/TableGen/ClangAttrEmitter.cpp | 2 +-
5 files changed, 34 insertions(+), 16 deletions(-)
diff --git a/clang/lib/AST/DeclPrinter.cpp b/clang/lib/AST/DeclPrinter.cpp
index 6afdb6cfccb142..c82d18be7741e5 100644
--- a/clang/lib/AST/DeclPrinter.cpp
+++ b/clang/lib/AST/DeclPrinter.cpp
@@ -119,7 +119,7 @@ namespace {
void printTemplateArguments(llvm::ArrayRef Args,
const TemplateParameterList *Params);
enum class AttrPosAsWritten { Default = 0, Left, Right };
-void
+bool
prettyPrintAttributes(const Decl *D,
AttrPosAsWritten Pos = AttrPosAsWritten::Default);
void prettyPrintPragmas(Decl *D);
@@ -252,10 +252,12 @@ static DeclPrinter::AttrPosAsWritten
getPosAsWritten(const Attr *A,
return DeclPrinter::AttrPosAsWritten::Right;
}
-void DeclPrinter::prettyPrintAttributes(const Decl *D,
+// returns true if an attribute was printed.
+bool DeclPrinter::prettyPrintAttributes(const Decl *D,
AttrPosAsWritten Pos /*=Default*/) {
+ bool hasPrinted = false;
if (Policy.PolishForDeclaration)
-return;
+return hasPrinted;
if (D->hasAttrs()) {
const AttrVec = D->getAttrs();
@@ -275,6 +277,7 @@ void DeclPrinter::prettyPrintAttributes(const Decl *D,
if (Pos != AttrPosAsWritten::Left)
Out << ' ';
A->printPretty(Out, Policy);
+ hasPrinted = true;
if (Pos == AttrPosAsWritten::Left)
Out << ' ';
}
@@ -282,6 +285,7 @@ void DeclPrinter::prettyPrintAttributes(const Decl *D,
}
}
}
+ return hasPrinted;
}
void DeclPrinter::prettyPrintPragmas(Decl *D) {
@@ -1060,12 +1064,15 @@ void DeclPrinter::VisitCXXRecordDecl(CXXRecordDecl *D) {
// FIXME: add printing of pragma attributes if required.
if (!Policy.SuppressSpecifiers && D->isModulePrivate())
Out << "__module_private__ ";
- Out << D->getKindName();
- prettyPrintAttributes(D);
+ Out << D->getKindName() << ' ';
- if (D->getIdentifier()) {
+ // FIXME: Move before printing the decl kind to match the behavior of the
+ // attribute printing for variables and function where they are printed
first.
+ if (prettyPrintAttributes(D, AttrPosAsWritten::Left))
Out << ' ';
+
+ if (D->getIdentifier()) {
if (auto *NNS = D->getQualifier())
NNS->print(Out, Policy);
Out << *D;
@@ -1082,13 +1089,10 @@ void DeclPrinter::VisitCXXRecordDecl(CXXRecordDecl *D) {
}
}
- if (D->hasDefinition()) {
-if (D->hasAttr()) {
- Out << " final";
-}
- }
+ prettyPrintAttributes(D, AttrPosAsWritten::Right);
if (D->isCompleteDefinition()) {
+Out << ' ';
// Print the base classes
if (D->getNumBases()) {
Out << " : ";
@@ -1109,15 +1113,16 @@ void DeclPrinter::VisitCXXRecordDecl(CXXRecordDecl *D) {
if (Base->isPackExpansion())
Out << "...";
+Out << ' ';
}
}
// Print the class definition
// FIXME: Doesn't print access specifiers, e.g., "public:"
if (Policy.TerseOutput) {
- Out << " {}";
+ Out << "{}";
} else {
- Out << " {\n";
+ Out << "{\n";
VisitDeclContext(D);
Indent() << "}";
}
diff --git a/clang/test/SemaCXX/cxx11-attr-print.cpp
b/clang/test/SemaCXX/cxx11-attr-print.cpp
index a169d1b4409b4d..2b084018bc0662 100644
--- a/clang/test/SemaCXX/cxx11-attr-print.cpp
+++ b/clang/test/SemaCXX/cxx11-attr-print.cpp
@@ -87,3 +87,8 @@ template struct S;
// CHECK: using Small2 {{\[}}[gnu::mode(byte)]] = int;
using Small2 [[gnu::mode(byte)]] = int;
+
+class FinalNonTemplate final {};
+// CHECK: class FinalNonTemplate final {
+template class FinalTemplate final {};
+// CHECK: template class FinalTemplate final {
diff --git a/clang/test/SemaHLSL/BuiltIns/RWBuffers.hlsl
b/clang/test/SemaHLSL/BuiltIns/RWBuffers.hlsl
index 7e79ae3bf005fc..b1a15c43191829 100644
--- a/clang/test/SemaHLSL/BuiltIns/RWBuffers.hlsl
+++ b/clang/test/SemaHLSL/BuiltIns/RWBuffers.hlsl
@@ -6,11 +6,11 @@ typedef vector float3;
RWBuffer Buffer;
// expected-error@+2 {{class template 'RWBuffer' requires template arguments}}
-// expected-note@*:* {{template declaration from hidden source: template
class RWBuffer final}}
+// expected-note@*:* {{template declaration from hidden source: template
class RWBuffer}}
RWBuffer BufferErr1;
//
[clang] Fix the double space and double attribute printing of the final keyword. (PR #88600)
kimgr wrote: > With .PolishForDeclaration=true, there are NO final specifiers (which is what > we want to produce forward decls in IWYU) This is actually a regression in this PR, and it breaks the clangd test added here: https://github.com/llvm/llvm-project/commit/9f57b65a272817752aa00e2fb94154e6eed1d0ec (the patch that originally led to double `final`s). https://github.com/llvm/llvm-project/pull/88600 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix the double space and double attribute printing of the final keyword. (PR #88600)
kimgr wrote:
Thanks!
Could you add this DeclPrinterTest unittest for regression?
```
TEST(DeclPrinter, TestTemplateFinal) {
ASSERT_TRUE(PrintedDeclCXX11Matches(
"template"
"class FinalTemplate final {};",
classTemplateDecl(hasName("FinalTemplate")).bind("id"),
"template class FinalTemplate final {}"));
}
```
This is my expectation for correct output, but the current branch seems to
disagree -- now there's two spaces between `final` and `{}`. Not sure if that's
a problem. IWYU has enough post-processing that it doesn't break us either way.
https://github.com/llvm/llvm-project/pull/88600
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix the double space and double attribute printing of the final keyword. (PR #88600)
@@ -6,11 +6,11 @@ typedef vector float3;
RWBuffer Buffer;
// expected-error@+2 {{class template 'RWBuffer' requires template arguments}}
-// expected-note@*:* {{template declaration from hidden source: template
class RWBuffer final}}
+// expected-note@*:* {{template declaration from hidden source: template
class RWBuffer}}
vgvassilev wrote:
We remove the `final` keyword here as it is modeled as an attribute and HLSL
calls `FinalAttr::CreateImplicit` to constrain the users from inheriting from
the builtin classes. We do not print implicit attributes. This used to work
because we double printed `final` once as an attribute and once as part of the
regular printing of CXXRecordDecls...
https://github.com/llvm/llvm-project/pull/88600
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix the double space and double attribute printing of the final keyword. (PR #88600)
https://github.com/vgvassilev updated
https://github.com/llvm/llvm-project/pull/88600
>From b43c52e0ab76815e745d474c40944b4de1a929ab Mon Sep 17 00:00:00 2001
From: Vassil Vassilev
Date: Sat, 13 Apr 2024 06:39:34 +
Subject: [PATCH] Fix the double space and double attribute printing of the
final keyword.
Fixes #56517.
---
clang/lib/AST/DeclPrinter.cpp | 30 -
clang/test/SemaCXX/cxx11-attr-print.cpp | 5
clang/test/SemaHLSL/BuiltIns/RWBuffers.hlsl | 4 +--
clang/utils/TableGen/ClangAttrEmitter.cpp | 2 +-
4 files changed, 25 insertions(+), 16 deletions(-)
diff --git a/clang/lib/AST/DeclPrinter.cpp b/clang/lib/AST/DeclPrinter.cpp
index 6afdb6cfccb142..46e5b7c47ef564 100644
--- a/clang/lib/AST/DeclPrinter.cpp
+++ b/clang/lib/AST/DeclPrinter.cpp
@@ -119,7 +119,7 @@ namespace {
void printTemplateArguments(llvm::ArrayRef Args,
const TemplateParameterList *Params);
enum class AttrPosAsWritten { Default = 0, Left, Right };
-void
+bool
prettyPrintAttributes(const Decl *D,
AttrPosAsWritten Pos = AttrPosAsWritten::Default);
void prettyPrintPragmas(Decl *D);
@@ -252,10 +252,12 @@ static DeclPrinter::AttrPosAsWritten
getPosAsWritten(const Attr *A,
return DeclPrinter::AttrPosAsWritten::Right;
}
-void DeclPrinter::prettyPrintAttributes(const Decl *D,
+// returns true if an attribute was printed.
+bool DeclPrinter::prettyPrintAttributes(const Decl *D,
AttrPosAsWritten Pos /*=Default*/) {
+ bool hasPrinted = false;
if (Policy.PolishForDeclaration)
-return;
+return hasPrinted;
if (D->hasAttrs()) {
const AttrVec = D->getAttrs();
@@ -275,6 +277,7 @@ void DeclPrinter::prettyPrintAttributes(const Decl *D,
if (Pos != AttrPosAsWritten::Left)
Out << ' ';
A->printPretty(Out, Policy);
+ hasPrinted = true;
if (Pos == AttrPosAsWritten::Left)
Out << ' ';
}
@@ -282,6 +285,7 @@ void DeclPrinter::prettyPrintAttributes(const Decl *D,
}
}
}
+ return hasPrinted;
}
void DeclPrinter::prettyPrintPragmas(Decl *D) {
@@ -1060,12 +1064,14 @@ void DeclPrinter::VisitCXXRecordDecl(CXXRecordDecl *D) {
// FIXME: add printing of pragma attributes if required.
if (!Policy.SuppressSpecifiers && D->isModulePrivate())
Out << "__module_private__ ";
- Out << D->getKindName();
- prettyPrintAttributes(D);
+ Out << D->getKindName() << ' ';
+
+ // FIXME: Move before printing the decl kind to match the behavior of the
+ // attribute printing for variables and function where they are printed
first.
+ prettyPrintAttributes(D, AttrPosAsWritten::Left);
if (D->getIdentifier()) {
-Out << ' ';
if (auto *NNS = D->getQualifier())
NNS->print(Out, Policy);
Out << *D;
@@ -1082,11 +1088,8 @@ void DeclPrinter::VisitCXXRecordDecl(CXXRecordDecl *D) {
}
}
- if (D->hasDefinition()) {
-if (D->hasAttr()) {
- Out << " final";
-}
- }
+ if (prettyPrintAttributes(D, AttrPosAsWritten::Right))
+Out << ' ';
if (D->isCompleteDefinition()) {
// Print the base classes
@@ -1114,10 +1117,11 @@ void DeclPrinter::VisitCXXRecordDecl(CXXRecordDecl *D) {
// Print the class definition
// FIXME: Doesn't print access specifiers, e.g., "public:"
+Out << ' ';
if (Policy.TerseOutput) {
- Out << " {}";
+ Out << "{}";
} else {
- Out << " {\n";
+ Out << "{\n";
VisitDeclContext(D);
Indent() << "}";
}
diff --git a/clang/test/SemaCXX/cxx11-attr-print.cpp
b/clang/test/SemaCXX/cxx11-attr-print.cpp
index a169d1b4409b4d..2b084018bc0662 100644
--- a/clang/test/SemaCXX/cxx11-attr-print.cpp
+++ b/clang/test/SemaCXX/cxx11-attr-print.cpp
@@ -87,3 +87,8 @@ template struct S;
// CHECK: using Small2 {{\[}}[gnu::mode(byte)]] = int;
using Small2 [[gnu::mode(byte)]] = int;
+
+class FinalNonTemplate final {};
+// CHECK: class FinalNonTemplate final {
+template class FinalTemplate final {};
+// CHECK: template class FinalTemplate final {
diff --git a/clang/test/SemaHLSL/BuiltIns/RWBuffers.hlsl
b/clang/test/SemaHLSL/BuiltIns/RWBuffers.hlsl
index 7e79ae3bf005fc..b1a15c43191829 100644
--- a/clang/test/SemaHLSL/BuiltIns/RWBuffers.hlsl
+++ b/clang/test/SemaHLSL/BuiltIns/RWBuffers.hlsl
@@ -6,11 +6,11 @@ typedef vector float3;
RWBuffer Buffer;
// expected-error@+2 {{class template 'RWBuffer' requires template arguments}}
-// expected-note@*:* {{template declaration from hidden source: template
class RWBuffer final}}
+// expected-note@*:* {{template declaration from hidden source: template
class RWBuffer}}
RWBuffer BufferErr1;
// expected-error@+2 {{too few template arguments for class template
'RWBuffer'}}
-// expected-note@*:* {{template declaration from hidden source: template
class RWBuffer final}}
+// expected-note@*:* {{template declaration from hidden
[clang] Fix the double space and double attribute printing of the final keyword. (PR #88600)
llvmbot wrote:
@llvm/pr-subscribers-clang
@llvm/pr-subscribers-hlsl
Author: Vassil Vassilev (vgvassilev)
Changes
Fixes #56517.
cc: @kimgr
---
Full diff: https://github.com/llvm/llvm-project/pull/88600.diff
4 Files Affected:
- (modified) clang/lib/AST/DeclPrinter.cpp (+18-13)
- (modified) clang/test/SemaCXX/cxx11-attr-print.cpp (+5)
- (modified) clang/test/SemaHLSL/BuiltIns/RWBuffers.hlsl (+2-2)
- (modified) clang/utils/TableGen/ClangAttrEmitter.cpp (+1-1)
``diff
diff --git a/clang/lib/AST/DeclPrinter.cpp b/clang/lib/AST/DeclPrinter.cpp
index 6afdb6cfccb142..c758f792b04a59 100644
--- a/clang/lib/AST/DeclPrinter.cpp
+++ b/clang/lib/AST/DeclPrinter.cpp
@@ -119,7 +119,7 @@ namespace {
void printTemplateArguments(llvm::ArrayRef Args,
const TemplateParameterList *Params);
enum class AttrPosAsWritten { Default = 0, Left, Right };
-void
+bool
prettyPrintAttributes(const Decl *D,
AttrPosAsWritten Pos = AttrPosAsWritten::Default);
void prettyPrintPragmas(Decl *D);
@@ -252,10 +252,12 @@ static DeclPrinter::AttrPosAsWritten
getPosAsWritten(const Attr *A,
return DeclPrinter::AttrPosAsWritten::Right;
}
-void DeclPrinter::prettyPrintAttributes(const Decl *D,
+// returns true if an attribute was printed.
+bool DeclPrinter::prettyPrintAttributes(const Decl *D,
AttrPosAsWritten Pos /*=Default*/) {
+ bool hasPrinted = false;
if (Policy.PolishForDeclaration)
-return;
+return hasPrinted;
if (D->hasAttrs()) {
const AttrVec = D->getAttrs();
@@ -275,6 +277,7 @@ void DeclPrinter::prettyPrintAttributes(const Decl *D,
if (Pos != AttrPosAsWritten::Left)
Out << ' ';
A->printPretty(Out, Policy);
+ hasPrinted = true;
if (Pos == AttrPosAsWritten::Left)
Out << ' ';
}
@@ -282,6 +285,7 @@ void DeclPrinter::prettyPrintAttributes(const Decl *D,
}
}
}
+ return hasPrinted;
}
void DeclPrinter::prettyPrintPragmas(Decl *D) {
@@ -1060,12 +1064,14 @@ void DeclPrinter::VisitCXXRecordDecl(CXXRecordDecl *D) {
// FIXME: add printing of pragma attributes if required.
if (!Policy.SuppressSpecifiers && D->isModulePrivate())
Out << "__module_private__ ";
- Out << D->getKindName();
- prettyPrintAttributes(D);
+ Out << D->getKindName() << ' ';
+
+ // FIXME: Move before printing the decl kind to match the behavior of the
+ // attribute printing for variables and function where they are printed
first.
+ prettyPrintAttributes(D, AttrPosAsWritten::Left);
if (D->getIdentifier()) {
-Out << ' ';
if (auto *NNS = D->getQualifier())
NNS->print(Out, Policy);
Out << *D;
@@ -1082,11 +1088,8 @@ void DeclPrinter::VisitCXXRecordDecl(CXXRecordDecl *D) {
}
}
- if (D->hasDefinition()) {
-if (D->hasAttr()) {
- Out << " final";
-}
- }
+ if (prettyPrintAttributes(D, AttrPosAsWritten::Right))
+Out << ' ';
if (D->isCompleteDefinition()) {
// Print the base classes
@@ -1114,10 +1117,12 @@ void DeclPrinter::VisitCXXRecordDecl(CXXRecordDecl *D) {
// Print the class definition
// FIXME: Doesn't print access specifiers, e.g., "public:"
+//if (!D->getIdentifier())
+ Out << ' ';
if (Policy.TerseOutput) {
- Out << " {}";
+ Out << "{}";
} else {
- Out << " {\n";
+ Out << "{\n";
VisitDeclContext(D);
Indent() << "}";
}
diff --git a/clang/test/SemaCXX/cxx11-attr-print.cpp
b/clang/test/SemaCXX/cxx11-attr-print.cpp
index a169d1b4409b4d..2b084018bc0662 100644
--- a/clang/test/SemaCXX/cxx11-attr-print.cpp
+++ b/clang/test/SemaCXX/cxx11-attr-print.cpp
@@ -87,3 +87,8 @@ template struct S;
// CHECK: using Small2 {{\[}}[gnu::mode(byte)]] = int;
using Small2 [[gnu::mode(byte)]] = int;
+
+class FinalNonTemplate final {};
+// CHECK: class FinalNonTemplate final {
+template class FinalTemplate final {};
+// CHECK: template class FinalTemplate final {
diff --git a/clang/test/SemaHLSL/BuiltIns/RWBuffers.hlsl
b/clang/test/SemaHLSL/BuiltIns/RWBuffers.hlsl
index 7e79ae3bf005fc..b1a15c43191829 100644
--- a/clang/test/SemaHLSL/BuiltIns/RWBuffers.hlsl
+++ b/clang/test/SemaHLSL/BuiltIns/RWBuffers.hlsl
@@ -6,11 +6,11 @@ typedef vector float3;
RWBuffer Buffer;
// expected-error@+2 {{class template 'RWBuffer' requires template arguments}}
-// expected-note@*:* {{template declaration from hidden source: template
class RWBuffer final}}
+// expected-note@*:* {{template declaration from hidden source: template
class RWBuffer}}
RWBuffer BufferErr1;
// expected-error@+2 {{too few template arguments for class template
'RWBuffer'}}
-// expected-note@*:* {{template declaration from hidden source: template
class RWBuffer final}}
+// expected-note@*:* {{template declaration from hidden source: template
class RWBuffer}}
RWBuffer<> BufferErr2;
[numthreads(1,1,1)]
diff
[clang] Fix the double space and double attribute printing of the final keyword. (PR #88600)
https://github.com/vgvassilev created
https://github.com/llvm/llvm-project/pull/88600
Fixes #56517.
cc: @kimgr
>From fe2622816265cf4977410d38dfd32d19df8eff5e Mon Sep 17 00:00:00 2001
From: Vassil Vassilev
Date: Sat, 13 Apr 2024 06:39:34 +
Subject: [PATCH] Fix the double space and double attribute printing of the
final keyword.
Fixes #56517.
---
clang/lib/AST/DeclPrinter.cpp | 31 -
clang/test/SemaCXX/cxx11-attr-print.cpp | 5
clang/test/SemaHLSL/BuiltIns/RWBuffers.hlsl | 4 +--
clang/utils/TableGen/ClangAttrEmitter.cpp | 2 +-
4 files changed, 26 insertions(+), 16 deletions(-)
diff --git a/clang/lib/AST/DeclPrinter.cpp b/clang/lib/AST/DeclPrinter.cpp
index 6afdb6cfccb142..c758f792b04a59 100644
--- a/clang/lib/AST/DeclPrinter.cpp
+++ b/clang/lib/AST/DeclPrinter.cpp
@@ -119,7 +119,7 @@ namespace {
void printTemplateArguments(llvm::ArrayRef Args,
const TemplateParameterList *Params);
enum class AttrPosAsWritten { Default = 0, Left, Right };
-void
+bool
prettyPrintAttributes(const Decl *D,
AttrPosAsWritten Pos = AttrPosAsWritten::Default);
void prettyPrintPragmas(Decl *D);
@@ -252,10 +252,12 @@ static DeclPrinter::AttrPosAsWritten
getPosAsWritten(const Attr *A,
return DeclPrinter::AttrPosAsWritten::Right;
}
-void DeclPrinter::prettyPrintAttributes(const Decl *D,
+// returns true if an attribute was printed.
+bool DeclPrinter::prettyPrintAttributes(const Decl *D,
AttrPosAsWritten Pos /*=Default*/) {
+ bool hasPrinted = false;
if (Policy.PolishForDeclaration)
-return;
+return hasPrinted;
if (D->hasAttrs()) {
const AttrVec = D->getAttrs();
@@ -275,6 +277,7 @@ void DeclPrinter::prettyPrintAttributes(const Decl *D,
if (Pos != AttrPosAsWritten::Left)
Out << ' ';
A->printPretty(Out, Policy);
+ hasPrinted = true;
if (Pos == AttrPosAsWritten::Left)
Out << ' ';
}
@@ -282,6 +285,7 @@ void DeclPrinter::prettyPrintAttributes(const Decl *D,
}
}
}
+ return hasPrinted;
}
void DeclPrinter::prettyPrintPragmas(Decl *D) {
@@ -1060,12 +1064,14 @@ void DeclPrinter::VisitCXXRecordDecl(CXXRecordDecl *D) {
// FIXME: add printing of pragma attributes if required.
if (!Policy.SuppressSpecifiers && D->isModulePrivate())
Out << "__module_private__ ";
- Out << D->getKindName();
- prettyPrintAttributes(D);
+ Out << D->getKindName() << ' ';
+
+ // FIXME: Move before printing the decl kind to match the behavior of the
+ // attribute printing for variables and function where they are printed
first.
+ prettyPrintAttributes(D, AttrPosAsWritten::Left);
if (D->getIdentifier()) {
-Out << ' ';
if (auto *NNS = D->getQualifier())
NNS->print(Out, Policy);
Out << *D;
@@ -1082,11 +1088,8 @@ void DeclPrinter::VisitCXXRecordDecl(CXXRecordDecl *D) {
}
}
- if (D->hasDefinition()) {
-if (D->hasAttr()) {
- Out << " final";
-}
- }
+ if (prettyPrintAttributes(D, AttrPosAsWritten::Right))
+Out << ' ';
if (D->isCompleteDefinition()) {
// Print the base classes
@@ -1114,10 +1117,12 @@ void DeclPrinter::VisitCXXRecordDecl(CXXRecordDecl *D) {
// Print the class definition
// FIXME: Doesn't print access specifiers, e.g., "public:"
+//if (!D->getIdentifier())
+ Out << ' ';
if (Policy.TerseOutput) {
- Out << " {}";
+ Out << "{}";
} else {
- Out << " {\n";
+ Out << "{\n";
VisitDeclContext(D);
Indent() << "}";
}
diff --git a/clang/test/SemaCXX/cxx11-attr-print.cpp
b/clang/test/SemaCXX/cxx11-attr-print.cpp
index a169d1b4409b4d..2b084018bc0662 100644
--- a/clang/test/SemaCXX/cxx11-attr-print.cpp
+++ b/clang/test/SemaCXX/cxx11-attr-print.cpp
@@ -87,3 +87,8 @@ template struct S;
// CHECK: using Small2 {{\[}}[gnu::mode(byte)]] = int;
using Small2 [[gnu::mode(byte)]] = int;
+
+class FinalNonTemplate final {};
+// CHECK: class FinalNonTemplate final {
+template class FinalTemplate final {};
+// CHECK: template class FinalTemplate final {
diff --git a/clang/test/SemaHLSL/BuiltIns/RWBuffers.hlsl
b/clang/test/SemaHLSL/BuiltIns/RWBuffers.hlsl
index 7e79ae3bf005fc..b1a15c43191829 100644
--- a/clang/test/SemaHLSL/BuiltIns/RWBuffers.hlsl
+++ b/clang/test/SemaHLSL/BuiltIns/RWBuffers.hlsl
@@ -6,11 +6,11 @@ typedef vector float3;
RWBuffer Buffer;
// expected-error@+2 {{class template 'RWBuffer' requires template arguments}}
-// expected-note@*:* {{template declaration from hidden source: template
class RWBuffer final}}
+// expected-note@*:* {{template declaration from hidden source: template
class RWBuffer}}
RWBuffer BufferErr1;
// expected-error@+2 {{too few template arguments for class template
'RWBuffer'}}
-// expected-note@*:* {{template declaration from hidden source: template
class RWBuffer
