Skip to content
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.

Sign up for GitHub

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

Fix the double space and double attribute printing of the final keyword. #88600

Merged
merged 2 commits into from
Apr 18, 2024
Merged

Fix the double space and double attribute printing of the final keyword. #88600

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9b2bb90
Fix the double space and double attribute printing of the final keyword.
vgvassilev Apr 13, 2024
2a54307
Address more review comments
vgvassilev Apr 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 21 additions & 15 deletions clang/lib/AST/DeclPrinter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ namespace {
void printTemplateArguments(llvm::ArrayRef<TemplateArgumentLoc> 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);
Expand Down Expand Up @@ -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 &Attrs = D->getAttrs();
for (auto *A : Attrs) {
if (A->isInherited() || A->isImplicit())
continue;
// Print out the keyword attributes, they aren't regular attributes.
if (Policy.PolishForDeclaration && !A->isKeywordAttribute())
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, I was about to suggest something like this, but didn't know isKeywordAttribute existed.

continue;
switch (A->getKind()) {
#define ATTR(X)
#define PRAGMA_SPELLING_ATTR(X) case attr::X:
Expand All @@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

see Out->tell() here, it is perhaps less error-prone here (that is, store the uint64_t value above, and see if it changed).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}

void DeclPrinter::prettyPrintPragmas(Decl *D) {
Expand Down Expand Up @@ -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;
Expand All @@ -1082,16 +1090,13 @@ void DeclPrinter::VisitCXXRecordDecl(CXXRecordDecl *D) {
}
}

if (D->hasDefinition()) {
if (D->hasAttr<FinalAttr>()) {
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())
Expand All @@ -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() << "}";
}
Expand Down
5 changes: 5 additions & 0 deletions clang/test/SemaCXX/cxx11-attr-print.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,3 +87,8 @@ template struct S<int>;

// CHECK: using Small2 {{\[}}[gnu::mode(byte)]] = int;
using Small2 [[gnu::mode(byte)]] = int;

class FinalNonTemplate final {};
// CHECK: class FinalNonTemplate final {
template <typename T> class FinalTemplate final {};
// CHECK: template <typename T> class FinalTemplate final {
4 changes: 2 additions & 2 deletions clang/test/SemaHLSL/BuiltIns/RWBuffers.hlsl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,11 @@ typedef vector<float, 3> float3;
RWBuffer<float3> Buffer;

// expected-error@+2 {{class template 'RWBuffer' requires template arguments}}
// expected-note@*:* {{template declaration from hidden source: template <class element_type> class RWBuffer final}}
// expected-note@*:* {{template declaration from hidden source: template <class element_type> class RWBuffer}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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...

RWBuffer BufferErr1;

// expected-error@+2 {{too few template arguments for class template 'RWBuffer'}}
// expected-note@*:* {{template declaration from hidden source: template <class element_type> class RWBuffer final}}
// expected-note@*:* {{template declaration from hidden source: template <class element_type> class RWBuffer}}
RWBuffer<> BufferErr2;

[numthreads(1,1,1)]
Expand Down
37 changes: 28 additions & 9 deletions clang/unittests/AST/DeclPrinterTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,16 +86,13 @@ ::testing::AssertionResult PrintedDeclCXX11Matches(StringRef Code,
ExpectedPrinted, "input.cc");
}

::testing::AssertionResult PrintedDeclCXX11Matches(
StringRef Code,
const DeclarationMatcher &NodeMatch,
StringRef ExpectedPrinted) {
::testing::AssertionResult
PrintedDeclCXX11Matches(StringRef Code, const DeclarationMatcher &NodeMatch,
StringRef ExpectedPrinted,
PrintingPolicyAdjuster PolicyModifier = nullptr) {
std::vector<std::string> 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(
Expand Down Expand Up @@ -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<typename T>\n"
"class FinalTemplate final {};",
classTemplateDecl(hasName("FinalTemplate")).bind("id"),
"template <typename T> 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<typename T>\n"
"class FinalTemplate final {};",
classTemplateDecl(hasName("FinalTemplate")).bind("id"),
"template <typename T> class FinalTemplate final {}",
[](PrintingPolicy &Policy) { Policy.PolishForDeclaration = true; }));
}
2 changes: 1 addition & 1 deletion clang/utils/TableGen/ClangAttrEmitter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1608,7 +1608,7 @@ writePrettyPrintFunction(const Record &R,
Prefix = "[";
Suffix = "]";
} else if (Variety == "Keyword") {
Prefix = " ";
Prefix = "";
Suffix = "";
} else if (Variety == "Pragma") {
Prefix = "#pragma ";
Expand Down
Loading