Rework the printing of attributes #87281
Conversation
vgvassilev
requested review from
AaronBallman,
alexey-bataev,
erichkeane and
Sirraide
llvmbot
added
clang
|
@giulianobelinassi, I could not select you for a reviewer but please take a look at the pull request. |
|
@llvm/pr-subscribers-clang Author: Vassil Vassilev (vgvassilev) ChangesCommit 46f3ade introduced a notion of printing the attributes on the left to improve the printing of attributes attached to variable declarations. The intent was to produce more GCC compatible code because clang tends to print the attributes on the right hand side which is not accepted by gcc. This approach has increased the complexity in tablegen and the attrubutes themselves as now the are supposed to know where they could appear. That lead to mishandling of the This patch takes an inspiration from the existing approach and tries to keep the position of the attributes as they were written. To do so we use simpler heuristic which checks if the source locations of the attribute precedes the declaration. If so, it is considered to be printed before the declaration. Fixes #87151 Patch is 31.51 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/87281.diff 16 Files Affected:
diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index 3e03e55612645b..bb77f62405fbec 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -324,13 +324,10 @@ class Spelling<string name, string variety, int version = 1> {
}
class GNU<string name> : Spelling<name, "GNU">;
-class Declspec<string name> : Spelling<name, "Declspec"> {
- bit PrintOnLeft = 1;
-}
+class Declspec<string name> : Spelling<name, "Declspec">;
class Microsoft<string name> : Spelling<name, "Microsoft">;
class CXX11<string namespace, string name, int version = 1>
: Spelling<name, "CXX11", version> {
- bit CanPrintOnLeft = 0;
string Namespace = namespace;
}
class C23<string namespace, string name, int version = 1>
@@ -596,12 +593,6 @@ class AttrSubjectMatcherAggregateRule<AttrSubject subject> {
def SubjectMatcherForNamed : AttrSubjectMatcherAggregateRule<Named>;
class Attr {
- // Specifies that when printed, this attribute is meaningful on the
- // 'left side' of the declaration.
- bit CanPrintOnLeft = 1;
- // Specifies that when printed, this attribute is required to be printed on
- // the 'left side' of the declaration.
- bit PrintOnLeft = 0;
// The various ways in which an attribute can be spelled in source
list<Spelling> Spellings;
// The things to which an attribute can appertain
@@ -937,7 +928,6 @@ def AVRSignal : InheritableAttr, TargetSpecificAttr<TargetAVR> {
}
def AsmLabel : InheritableAttr {
- let CanPrintOnLeft = 0;
let Spellings = [CustomKeyword<"asm">, CustomKeyword<"__asm__">];
let Args = [
// Label specifies the mangled name for the decl.
@@ -1534,7 +1524,6 @@ def AllocSize : InheritableAttr {
}
def EnableIf : InheritableAttr {
- let CanPrintOnLeft = 0;
// Does not have a [[]] spelling because this attribute requires the ability
// to parse function arguments but the attribute is not written in the type
// position.
@@ -3149,7 +3138,6 @@ def Unavailable : InheritableAttr {
}
def DiagnoseIf : InheritableAttr {
- let CanPrintOnLeft = 0;
// Does not have a [[]] spelling because this attribute requires the ability
// to parse function arguments but the attribute is not written in the type
// position.
diff --git a/clang/include/clang/Basic/CMakeLists.txt b/clang/include/clang/Basic/CMakeLists.txt
index 7d53c751c13ac4..2ef6ddc68f4bf3 100644
--- a/clang/include/clang/Basic/CMakeLists.txt
+++ b/clang/include/clang/Basic/CMakeLists.txt
@@ -31,16 +31,6 @@ clang_tablegen(AttrList.inc -gen-clang-attr-list
SOURCE Attr.td
TARGET ClangAttrList)
-clang_tablegen(AttrLeftSideCanPrintList.inc -gen-clang-attr-can-print-left-list
- -I ${CMAKE_CURRENT_SOURCE_DIR}/../../
- SOURCE Attr.td
- TARGET ClangAttrCanPrintLeftList)
-
-clang_tablegen(AttrLeftSideMustPrintList.inc -gen-clang-attr-must-print-left-list
- -I ${CMAKE_CURRENT_SOURCE_DIR}/../../
- SOURCE Attr.td
- TARGET ClangAttrMustPrintLeftList)
-
clang_tablegen(AttrSubMatchRulesList.inc -gen-clang-attr-subject-match-rule-list
-I ${CMAKE_CURRENT_SOURCE_DIR}/../../
SOURCE Attr.td
diff --git a/clang/lib/AST/DeclPrinter.cpp b/clang/lib/AST/DeclPrinter.cpp
index edbcdfe4d55bc9..1e62d397db92f8 100644
--- a/clang/lib/AST/DeclPrinter.cpp
+++ b/clang/lib/AST/DeclPrinter.cpp
@@ -21,6 +21,7 @@
#include "clang/AST/ExprCXX.h"
#include "clang/AST/PrettyPrinter.h"
#include "clang/Basic/Module.h"
+#include "clang/Basic/SourceManager.h"
#include "llvm/Support/raw_ostream.h"
using namespace clang;
@@ -49,18 +50,6 @@ namespace {
void PrintObjCTypeParams(ObjCTypeParamList *Params);
- enum class AttrPrintLoc {
- None = 0,
- Left = 1,
- Right = 2,
- Any = Left | Right,
-
- LLVM_MARK_AS_BITMASK_ENUM(/*DefaultValue=*/Any)
- };
-
- void prettyPrintAttributes(Decl *D, raw_ostream &out,
- AttrPrintLoc loc = AttrPrintLoc::Any);
-
public:
DeclPrinter(raw_ostream &Out, const PrintingPolicy &Policy,
const ASTContext &Context, unsigned Indentation = 0,
@@ -129,11 +118,13 @@ namespace {
const TemplateParameterList *Params);
void printTemplateArguments(llvm::ArrayRef<TemplateArgumentLoc> Args,
const TemplateParameterList *Params);
-
- inline void prettyPrintAttributes(Decl *D) {
- prettyPrintAttributes(D, Out);
- }
-
+ enum class AttrPosAsWritten {
+ Unknown = 0,
+ Left = 1,
+ Right = 2,
+ };
+ void prettyPrintAttributes(const Decl *D,
+ AttrPosAsWritten P = AttrPosAsWritten::Unknown);
void prettyPrintPragmas(Decl *D);
void printDeclType(QualType T, StringRef DeclName, bool Pack = false);
};
@@ -250,87 +241,43 @@ raw_ostream& DeclPrinter::Indent(unsigned Indentation) {
return Out;
}
-// For CLANG_ATTR_LIST_CanPrintOnLeft macro.
-#include "clang/Basic/AttrLeftSideCanPrintList.inc"
-
-// For CLANG_ATTR_LIST_PrintOnLeft macro.
-#include "clang/Basic/AttrLeftSideMustPrintList.inc"
-
-static bool canPrintOnLeftSide(attr::Kind kind) {
-#ifdef CLANG_ATTR_LIST_CanPrintOnLeft
- switch (kind) {
- CLANG_ATTR_LIST_CanPrintOnLeft
- return true;
- default:
- return false;
- }
-#else
- return false;
-#endif
-}
-
-static bool canPrintOnLeftSide(const Attr *A) {
- if (A->isStandardAttributeSyntax())
- return false;
+static DeclPrinter::AttrPosAsWritten getAttrPosAsWritten(const Attr *A,
+ const Decl *D) {
+ SourceLocation ALoc = A->getLoc();
+ SourceLocation DLoc = D->getLocation();
+ const ASTContext &C = D->getASTContext();
+ if (ALoc.isInvalid() || DLoc.isInvalid())
+ return DeclPrinter::AttrPosAsWritten::Unknown;
- return canPrintOnLeftSide(A->getKind());
-}
+ if (C.getSourceManager().isBeforeInTranslationUnit(ALoc, DLoc))
+ return DeclPrinter::AttrPosAsWritten::Left;
-static bool mustPrintOnLeftSide(attr::Kind kind) {
-#ifdef CLANG_ATTR_LIST_PrintOnLeft
- switch (kind) {
- CLANG_ATTR_LIST_PrintOnLeft
- return true;
- default:
- return false;
- }
-#else
- return false;
-#endif
+ return DeclPrinter::AttrPosAsWritten::Right;
}
-static bool mustPrintOnLeftSide(const Attr *A) {
- if (A->isDeclspecAttribute())
- return true;
-
- return mustPrintOnLeftSide(A->getKind());
-}
-
-void DeclPrinter::prettyPrintAttributes(Decl *D, llvm::raw_ostream &Out,
- AttrPrintLoc Loc) {
+void DeclPrinter::prettyPrintAttributes(const Decl *D,
+ AttrPosAsWritten P /*=Unknown*/) {
if (Policy.PolishForDeclaration)
return;
if (D->hasAttrs()) {
- AttrVec &Attrs = D->getAttrs();
+ const AttrVec &Attrs = D->getAttrs();
for (auto *A : Attrs) {
if (A->isInherited() || A->isImplicit())
continue;
-
- AttrPrintLoc AttrLoc = AttrPrintLoc::Right;
- if (mustPrintOnLeftSide(A)) {
- // If we must always print on left side (e.g. declspec), then mark as
- // so.
- AttrLoc = AttrPrintLoc::Left;
- } else if (canPrintOnLeftSide(A)) {
- // For functions with body defined we print the attributes on the left
- // side so that GCC accept our dumps as well.
- if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D);
- FD && FD->isThisDeclarationADefinition())
- // In case Decl is a function with a body, then attrs should be print
- // on the left side.
- AttrLoc = AttrPrintLoc::Left;
-
- // In case it is a variable declaration with a ctor, then allow
- // printing on the left side for readbility.
- else if (const VarDecl *VD = dyn_cast<VarDecl>(D);
- VD && VD->getInit() &&
- VD->getInitStyle() == VarDecl::CallInit)
- AttrLoc = AttrPrintLoc::Left;
+ switch (A->getKind()) {
+#define ATTR(X)
+#define PRAGMA_SPELLING_ATTR(X) case attr::X:
+#include "clang/Basic/AttrList.inc"
+ break;
+ default:
+ if (P == AttrPosAsWritten::Unknown || P == getAttrPosAsWritten(A, D)) {
+ A->printPretty(Out, Policy);
+ if (P == AttrPosAsWritten::Left)
+ Out << " ";
+ }
+ break;
}
- // Only print the side matches the user requested.
- if ((Loc & AttrLoc) != AttrPrintLoc::None)
- A->printPretty(Out, Policy);
}
}
}
@@ -691,8 +638,10 @@ static void MaybePrintTagKeywordIfSupressingScopes(PrintingPolicy &Policy,
void DeclPrinter::VisitFunctionDecl(FunctionDecl *D) {
if (!D->getDescribedFunctionTemplate() &&
- !D->isFunctionTemplateSpecialization())
+ !D->isFunctionTemplateSpecialization()) {
prettyPrintPragmas(D);
+ prettyPrintAttributes(D, AttrPosAsWritten::Left);
+ }
if (D->isFunctionTemplateSpecialization())
Out << "template<> ";
@@ -702,22 +651,6 @@ void DeclPrinter::VisitFunctionDecl(FunctionDecl *D) {
printTemplateParameters(D->getTemplateParameterList(I));
}
- std::string LeftsideAttrs;
- llvm::raw_string_ostream LSAS(LeftsideAttrs);
-
- prettyPrintAttributes(D, LSAS, AttrPrintLoc::Left);
-
- // prettyPrintAttributes print a space on left side of the attribute.
- if (LeftsideAttrs[0] == ' ') {
- // Skip the space prettyPrintAttributes generated.
- LeftsideAttrs.erase(0, LeftsideAttrs.find_first_not_of(' '));
-
- // Add a single space between the attribute and the Decl name.
- LSAS << ' ';
- }
-
- Out << LeftsideAttrs;
-
CXXConstructorDecl *CDecl = dyn_cast<CXXConstructorDecl>(D);
CXXConversionDecl *ConversionDecl = dyn_cast<CXXConversionDecl>(D);
CXXDeductionGuideDecl *GuideDecl = dyn_cast<CXXDeductionGuideDecl>(D);
@@ -883,7 +816,7 @@ void DeclPrinter::VisitFunctionDecl(FunctionDecl *D) {
Ty.print(Out, Policy, Proto);
}
- prettyPrintAttributes(D, Out, AttrPrintLoc::Right);
+ prettyPrintAttributes(D, AttrPosAsWritten::Right);
if (D->isPureVirtual())
Out << " = 0";
@@ -976,27 +909,12 @@ void DeclPrinter::VisitLabelDecl(LabelDecl *D) {
void DeclPrinter::VisitVarDecl(VarDecl *D) {
prettyPrintPragmas(D);
+ prettyPrintAttributes(D, AttrPosAsWritten::Left);
+
if (const auto *Param = dyn_cast<ParmVarDecl>(D);
Param && Param->isExplicitObjectParameter())
Out << "this ";
- std::string LeftSide;
- llvm::raw_string_ostream LeftSideStream(LeftSide);
-
- // Print attributes that should be placed on the left, such as __declspec.
- prettyPrintAttributes(D, LeftSideStream, AttrPrintLoc::Left);
-
- // prettyPrintAttributes print a space on left side of the attribute.
- if (LeftSide[0] == ' ') {
- // Skip the space prettyPrintAttributes generated.
- LeftSide.erase(0, LeftSide.find_first_not_of(' '));
-
- // Add a single space between the attribute and the Decl name.
- LeftSideStream << ' ';
- }
-
- Out << LeftSide;
-
QualType T = D->getTypeSourceInfo()
? D->getTypeSourceInfo()->getType()
: D->getASTContext().getUnqualifiedObjCPointerType(D->getType());
@@ -1029,21 +947,16 @@ void DeclPrinter::VisitVarDecl(VarDecl *D) {
}
}
- StringRef Name;
-
- Name = (isa<ParmVarDecl>(D) && Policy.CleanUglifiedParameters &&
- D->getIdentifier())
- ? D->getIdentifier()->deuglifiedName()
- : D->getName();
-
if (!Policy.SuppressTagKeyword && Policy.SuppressScope &&
!Policy.SuppressUnwrittenScope)
MaybePrintTagKeywordIfSupressingScopes(Policy, T, Out);
- printDeclType(T, Name);
- // Print the attributes that should be placed right before the end of the
- // decl.
- prettyPrintAttributes(D, Out, AttrPrintLoc::Right);
+ printDeclType(T, (isa<ParmVarDecl>(D) && Policy.CleanUglifiedParameters &&
+ D->getIdentifier())
+ ? D->getIdentifier()->deuglifiedName()
+ : D->getName());
+
+ prettyPrintAttributes(D, AttrPosAsWritten::Right);
Expr *Init = D->getInit();
if (!Policy.SuppressInitializers && Init) {
diff --git a/clang/test/AST/ast-print-attr-knr.c b/clang/test/AST/ast-print-attr-knr.c
deleted file mode 100644
index d7a53f958669f3..00000000000000
--- a/clang/test/AST/ast-print-attr-knr.c
+++ /dev/null
@@ -1,17 +0,0 @@
-// This file contain tests for attribute arguments on K&R functions.
-
-// RUN: %clang_cc1 -ast-print -x c -std=c89 -fms-extensions %s -o - | FileCheck %s
-
-// CHECK: int knr(i)
-// CHECK-NEXT: int i __attribute__((unused));
-// CHECK-NEXT: {
-// CHECK-NEXT: return 0;
-// CHECK-NEXT: }
-int knr(i) int i __attribute__((unused)); { return 0; }
-
-// CHECK: __attribute__((unused)) int knr2(i)
-// CHECK-NEXT: int i;
-// CHECK-NEXT: {
-// CHECK-NEXT: return 0;
-// CHECK-NEXT: }
-__attribute__((unused)) int knr2(i) int i; { return 0; }
diff --git a/clang/test/AST/ast-print-method-decl.cpp b/clang/test/AST/ast-print-method-decl.cpp
index 75dea0cac16be1..cb5d10096381a9 100644
--- a/clang/test/AST/ast-print-method-decl.cpp
+++ b/clang/test/AST/ast-print-method-decl.cpp
@@ -94,7 +94,7 @@ struct DefMethodsWithoutBody {
// CHECK-NEXT: DefMethodsWithoutBody() = default;
~DefMethodsWithoutBody() = default;
- // CHECK-NEXT: __attribute__((alias("X"))) void m1();
+ // CHECK-NEXT: void m1() __attribute__((alias("X")));
void m1() __attribute__((alias("X")));
// CHECK-NEXT: };
diff --git a/clang/test/AST/ast-print-no-sanitize.cpp b/clang/test/AST/ast-print-no-sanitize.cpp
index 4ff97190955add..a5ada8246f0c0b 100644
--- a/clang/test/AST/ast-print-no-sanitize.cpp
+++ b/clang/test/AST/ast-print-no-sanitize.cpp
@@ -4,4 +4,4 @@ void should_not_crash_1() __attribute__((no_sanitize_memory));
[[clang::no_sanitize_memory]] void should_not_crash_2();
// CHECK: void should_not_crash_1() __attribute__((no_sanitize("memory")));
-// CHECK: void should_not_crash_2() {{\[\[}}clang::no_sanitize("memory"){{\]\]}};
+// CHECK: {{\[\[}}clang::no_sanitize("memory"){{\]\]}} void should_not_crash_2();
diff --git a/clang/test/Analysis/scopes-cfg-output.cpp b/clang/test/Analysis/scopes-cfg-output.cpp
index 4eb8967e373516..5e6706602d4564 100644
--- a/clang/test/Analysis/scopes-cfg-output.cpp
+++ b/clang/test/Analysis/scopes-cfg-output.cpp
@@ -1469,7 +1469,7 @@ void test_cleanup_functions2(int m) {
// CHECK: [B1]
// CHECK-NEXT: 1: CFGScopeBegin(f)
// CHECK-NEXT: 2: (CXXConstructExpr, [B1.3], F)
-// CHECK-NEXT: 3: __attribute__((cleanup(cleanup_F))) F f;
+// CHECK-NEXT: 3: F f __attribute__((cleanup(cleanup_F)));
// CHECK-NEXT: 4: CleanupFunction (cleanup_F)
// CHECK-NEXT: 5: [B1.3].~F() (Implicit destructor)
// CHECK-NEXT: 6: CFGScopeEnd(f)
diff --git a/clang/test/OpenMP/assumes_codegen.cpp b/clang/test/OpenMP/assumes_codegen.cpp
index 6a5871c303aade..4db4327e85490c 100644
--- a/clang/test/OpenMP/assumes_codegen.cpp
+++ b/clang/test/OpenMP/assumes_codegen.cpp
@@ -67,36 +67,36 @@ int lambda_outer() {
}
#pragma omp end assumes
-// AST: __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp"))) void foo() {
+// AST: void foo() __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp"))) {
// AST-NEXT: }
// AST-NEXT: class BAR {
// AST-NEXT: public:
// AST-NEXT: __attribute__((assume("ompx_range_bar_only"))) __attribute__((assume("ompx_range_bar_only_2"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp"))) BAR() {
// AST-NEXT: }
-// AST-NEXT: __attribute__((assume("ompx_range_bar_only"))) __attribute__((assume("ompx_range_bar_only_2"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp"))) void bar1() {
+// AST-NEXT: __attribute__((assume("ompx_range_bar_only"))) __attribute__((assume("ompx_range_bar_only_2"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp"))) void bar1() {
// AST-NEXT: }
// AST-NEXT: __attribute__((assume("ompx_range_bar_only"))) __attribute__((assume("ompx_range_bar_only_2"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp"))) static void bar2() {
// AST-NEXT: }
// AST-NEXT: };
-// AST-NEXT: __attribute__((assume("ompx_range_bar_only"))) __attribute__((assume("ompx_range_bar_only_2"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp"))) void bar() {
+// AST-NEXT: __attribute__((assume("ompx_range_bar_only"))) __attribute__((assume("ompx_range_bar_only_2"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp"))) void bar() {
// AST-NEXT: BAR b;
// AST-NEXT: }
-// AST-NEXT: void baz() __attribute__((assume("ompx_1234"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp")));
+// AST-NEXT: __attribute__((assume("ompx_1234"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp"))) void baz();
// AST-NEXT: template <typename T> class BAZ {
// AST-NEXT: public:
// AST-NEXT: __attribute__((assume("ompx_1234"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp"))) BAZ<T>() {
// AST-NEXT: }
-// AST-NEXT: __attribute__((assume("ompx_1234"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp"))) void baz1() {
+// AST-NEXT: __attribute__((assume("ompx_1234"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp"))) void baz1() {
// AST-NEXT: }
-// AST-NEXT: __attribute__((assume("ompx_1234"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp"))) static void baz2() {
+// AST-NEXT: __attribute__((assume("ompx_1234"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp"))) static void baz2() {
// AST-NEXT: }
// AST-NEXT: };
// AST-NEXT: template<> class BAZ<float> {
// AST-NEXT: public:
-// AST-NEXT: __attribute__((assume("ompx_1234"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp"))) BAZ() {
+// AST-NEXT: __attribute__((assume("ompx_1234"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp"))) BAZ() {
// AST-NEXT: }
-// AST-NEXT: void baz1() __attribute__((assume("ompx_1234"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp")));
-// AST-NEXT: static void baz2() __attribute__((assume("ompx_1234"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp")));
+// AST-NEXT: __attribute__((assume("ompx_1234"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_a...
[truncated]
|
vgvassilev
force-pushed
the
attr-location
branch
from
4686e8f to
4a86afb
Compare
|
I am not sure how I feel about dropping the canPrintOnRight/printOnRight logic. We use it as a fallback when the SourceRange of a function is unreliable, otherwise we always copy and paste the code the user wrote. Regardless of that I will check for fallbacks on clang-extract once I get it to link with this branch. |
Which cases do you have in mind? In theory we can always print the ones we do not know on the left. I did not see a benefit for keeping the added complexity. Can come up with an example we cannot currently support?
If you can feed more examples that we should support that'd be great. |
Alright, so I got clang-extract to work on your branch and the attribute printing is working fine. which is enough for my use cases. Notice, however, that |
Awesome to hear! Thank you!
This is the annoying part I hoped to get fixed. There is one extra leading space, too... :( |
If you take a look at the diff you will see that there was code to check for an extra leading space and removing it. Perhaps you could add a boolean parameter to Or just keep the old logic and remove it from the string after it is print. |
vgvassilev
force-pushed
the
attr-location
branch
from
9a05fe3 to
1eb8bba
Compare
|
@erichkeane, I think I addressed your comments, can you take a look, @giulianobelinassi, I think I found the one off space. Can you check if your case has the right amount of spaces? |
vgvassilev
force-pushed
the
attr-location
branch
2 times, most recently
from
0c6526a to
a0c69ea
Compare
| @@ -17,23 +17,23 @@ template <typename T> | |||
| struct S { | |||
| int a; | |||
| // CHECK: template <typename T> struct S { | |||
| // CHECK: __attribute__((assume("ompx_global_assumption"))) void foo() { | |||
| // CHECK: void foo() __attribute__((assume("ompx_global_assumption"))) { | |||
There was a problem hiding this comment.
What causes these to swap sides?
There was a problem hiding this comment.
I think that attribute is added by #pragma omp parallel which appears after the definition of the function in terms of source locations. I thought we should probably filter out attributes that came from pragmas, however, it seems that we do not retain that information in the attribute and it says "gnu" rather than "pragma"...
There was a problem hiding this comment.
Is it perhaps set as "isImplicit"? I wonder if we could filter out based on that?
There was a problem hiding this comment.
It is not implicit - we don't print them. I looked and could not find anything that could distinguish the ones that come from pragmas. I briefly looked to see if we can add this in but seems some more work to be able to ask ::isOMPAttr in tablegen. Generally that's a good improvement but it has a different timeline to implement than that PR which is meant to be a hot fix...
There was a problem hiding this comment.
@alexey-bataev : Comments/concerns?
| // CHECK-NEXT: return a + b; | ||
| // CHECK-NEXT: } | ||
| #pragma omp declare simd uniform(this, a) linear(val(b): a) | ||
| __attribute__((cold)) int add(int a, int b) { return a + b; } | ||
| int add(int a, int b) __attribute__((cold)) { return a + b; } |
There was a problem hiding this comment.
Why did you change where the attribute was placed here? is there a problem leaving it on the left?
There was a problem hiding this comment.
Same reason I mentioned above. It's introduced by a pragma whose source location we take.
vgvassilev
force-pushed
the
attr-location
branch
from
a0c69ea to
175631c
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
vgvassilev
force-pushed
the
attr-location
branch
from
175631c to
0add10f
Compare
| if (Policy.PolishForDeclaration) | ||
| return; | ||
|
|
||
| if (D->hasAttrs()) { | ||
| AttrVec &Attrs = D->getAttrs(); | ||
| const AttrVec &Attrs = D->getAttrs(); |
There was a problem hiding this comment.
nit: I'd like an assert here to make sure Pos != Unknown (or at the beginning of hte function).
There was a problem hiding this comment.
I added an assert in the if-stmt. Is that what you meant?
There was a problem hiding this comment.
I see that, it does what I want, yes.
clang/lib/AST/DeclPrinter.cpp
Outdated
| break; | ||
| default: | ||
| AttrPosAsWritten APos = getPosAsWritten(A, D); | ||
| assert(APos != AttrPosAsWritten::Unknown && "Implicit attribute!"); |
There was a problem hiding this comment.
Isn't this the inverse? APos CAN be Unknown, it is Default that we need to make sure it isn't.
There was a problem hiding this comment.
APos cannot be unknown because the loop continues if (A->isInherited() || A->isImplicit()).
There was a problem hiding this comment.
IS that the only case where either the attribute or declaration don't have a valid source location? I find myself wondering about future attributes with a deduction guide/etc (or some other declaration that we represent as an AST node that gets generated out of thin air, that also might get an attribute from a spelled thing).
There was a problem hiding this comment.
Also, probably should make sure APos isn't 'default' either via assert.
There was a problem hiding this comment.
I think if APos is unknown, then there's a bug elsewhere. You can create attributes manually without using CreateImplicit() and so it's possible to add attributes with invalid source locations. For example, I spotted:
llvm-project/clang/lib/Sema/SemaAPINotes.cpp
Line 586 in ea88bb1
llvm-project/clang/lib/Sema/SemaDecl.cpp
Line 6884 in ea88bb1
So I think the assertion is reasonable but should probably come with a comment explaining why it might be triggered and what to do if it is (switch things to using CreateImplicit()).
| default: | ||
| AttrPosAsWritten APos = getPosAsWritten(A, D); | ||
| assert(APos != AttrPosAsWritten::Unknown && "Implicit attribute!"); | ||
| if (Pos == AttrPosAsWritten::Default || Pos == APos) { |
There was a problem hiding this comment.
In the case we are printing Left and Right, where do attributes we can't figure out based on location get printed? That is, where do Unknown location attributes get printed?
There was a problem hiding this comment.
If we have not specified Left or Right it prints them in their default position which seems predominantly on the right. In theory, we can rework all calls to prettyPrintAttributes and remove the need of Default because we will have:
printing_logic;
printAttributes(left);
more_printing_logic;
printAttributes(right);
That's doable but seemed outside of the scope of this PR since I am hoping to get it merged asap...
vgvassilev
force-pushed
the
attr-location
branch
from
4665151 to
cb2d7fb
Compare
vgvassilev
force-pushed
the
attr-location
branch
from
a84f609 to
fd21bb2
Compare
|
@erichkeane, thank you. What's the process of including this in the next release? |
After CI is complete, you can click "Squash and Merge" below (if you cannot, let us know and someone can do it for you), and it'll be included in the 19.1 release this summer. |
I have commit access. I want this to be part of the 18.x releases as that breaks our downstream clients. |
Ah, hmm... I am not sure this qualifies for inclusion in the current release branch. Perhaps @AaronBallman can comment here. |
|
#87151 has more context. |
This reverts commit a30662f due to bot failures.
Original commit message: " Commit 46f3ade introduced a notion of printing the attributes on the left to improve the printing of attributes attached to variable declarations. The intent was to produce more GCC compatible code because clang tends to print the attributes on the right hand side which is not accepted by gcc. This approach has increased the complexity in tablegen and the attrubutes themselves as now the are supposed to know where they could appear. That lead to mishandling of the `override` keyword which is modelled as an attribute in clang. This patch takes an inspiration from the existing approach and tries to keep the position of the attributes as they were written. To do so we use simpler heuristic which checks if the source locations of the attribute precedes the declaration. If so, it is considered to be printed before the declaration. Fixes #87151 " The reason for the bot breakage is that attributes coming from ApiNotes are not marked implicit even though they do not have source locations. This caused an assert to trigger. This patch forces attributes with no source location information to be printed on the left. That change is consistent to the overall intent of the change to increase the chances for attributes to compile across toolchains and at the same time the produced code to be as close as possible to the one written by the user.
I'm not particularly comfortable putting this into 18.x; we should only be pushing very safe fixes to regressions and this one doesn't really qualify. It's debatable whether it's a regression (it sort of is, sort of isn't), but the changes are also relatively involved. I'd feel more comfortable with this in 19.x so we have more time to find and fix remaining edge cases. |
|
Ok. Fair enough. |
How about this single-line fix? It would fix the override regression for now. |
|
Great idea! That’d make sense to me. |
|
We would like this fixed in 18.1 as well. We are expanding to support C++ and we will hit this bug at some point. |
|
Maybe you can open a PR against the branch? |
|
And |
Sorry, but I can only do this tomorrow. Feel free to open the PR if you need it immediately.
Why not? |
"broken" is a bit subjective in this case -- this isn't a user-facing feature and it's always been best-effort. I realize a few folks would like to elevate it to be something more than best-effort, but it's still unclear how that works in practice and whether it's worth the ongoing maintenance burdens in upstream. Fixing a few corner cases does improve things and the fix is trivial to verify for correctness, so that's why I'm not strongly opposed. I'm just not certain it meets the usual criteria for inclusion in a dot release. |
|
A note from left field: I think this PR broke the IWYU test suite. We use -template <typename T> class FinalTemplate;
+template <typename T> class FinalTemplate;So a spurious extra space. We would be grateful if this didn't make the 18.x branches, because we've already released an 18.x-compatible version of IWYU, and it would be a shame if that release had a failing test suite when built against later 18.x Clang. I'll work around this on our latest mainline. |
Ooof, I think that DOES decide it for me. @vgvassilev : can you look into that extra space to see if you can track it down? |
|
@erichkeane Thanks! Let me just see if I can bisect it to this commit; I don't have any evidence yet, this PR was just the only significant change to |
|
I can confirm that the double space comes from this PR; diff --git a/clang/unittests/AST/DeclPrinterTest.cpp b/clang/unittests/AST/DeclPrinterTest.cpp
index c24e442621c9..c2d02e74a62c 100644
--- a/clang/unittests/AST/DeclPrinterTest.cpp
+++ b/clang/unittests/AST/DeclPrinterTest.cpp
@@ -1555,3 +1555,11 @@ TEST(DeclPrinter, VarDeclWithInitializer) {
PrintedDeclCXX17Matches("void foo() {int arr[42]; for(int a : arr);}",
namedDecl(hasName("a")).bind("id"), "int a"));
}
+
+TEST(DeclPrinter, TestTemplateFinal) {
+ ASSERT_TRUE(PrintedDeclCXX11Matches(
+ "template<typename T>\n"
+ "class FinalTemplate final {};",
+ classTemplateDecl(hasName("FinalTemplate")).bind("id"),
+ "template <typename T> class final FinalTemplate final {}"));
+}fails with: (edited so it's easier to see the diff). It passes after I revert the Again, from my point of view, this "bug" is fine on mainline, we can work around it. But it would be nice if this patch was not backported to 18 as it breaks our corresponding release. |
|
(obtw, the double |
@kimgr, than you for the report. I can reproduce the double space. However, do you expect that test of yours to be valid C++? As written it seems not. |
|
@vgvassilev I did expect the input to be valid, yes: Is it not? |
The snippet as visualized in github seems to have one too many |
|
Ah, that's the expected output -- I can't do anything about that :). See #56517. |
Commit 46f3ade introduced a notion of printing the attributes on the left to improve the printing of attributes attached to variable declarations. The intent was to produce more GCC compatible code because clang tends to print the attributes on the right hand side which is not accepted by gcc.
This approach has increased the complexity in tablegen and the attrubutes themselves as now the are supposed to know where they could appear. That lead to mishandling of the
overridekeyword which is modelled as an attribute in clang.This patch takes an inspiration from the existing approach and tries to keep the position of the attributes as they were written. To do so we use simpler heuristic which checks if the source locations of the attribute precedes the declaration. If so, it is considered to be printed before the declaration.
Fixes #87151