jdenny created this revision. jdenny added reviewers: rsmith, nik, jbcoe, aaron.ballman, hfinkel.
For example, given: void fn() { struct T *p0; struct T { int i; } *p1; } -ast-print produced: void fn() { struct T { int i; } *p0; struct T { int i; } *p1; } Compiling that fails with a redefinition error. Details: For a tag specifier (that is, struct/union/class/enum used as a type specifier in a declaration) that was also a tag declaration (that is, first occurrence of the tag) or tag redeclaration (that is, later occurrence that specifies attributes or a member list), clang printed the tag specifier as either (1) the full tag definition if one existed, or (2) the first tag declaration otherwise. As in the above example, redefinition errors were sometimes introduced. Even when that was impossible because no member list was ever specified, attributes were sometimes lost, thus changing semantics and diagnostics. This patch fixes a major culprit for these problems. It does so by replacing PrintingPolicy's IncludeTagDefinition, which triggered printing of the member list, attributes, etc. for a tag specifier by using a tag (re)declaration selected as described above. The replacement is TagSpecifierAs, which triggers the same thing except it specifies the tag (re)declaration actually recorded by the parser for that tag specifier. I have two concerns about this patch: 1. TagSpecifierAs expands sizeof(PrintingPolicy) from 8 to 16 bytes. My concern is the comments on PrintingPolicy about how PrintingPolicy is intended to be small. My guess it that 16 bytes is still small enough, but perhaps Richard Smith, who wrote that comment, knows better. 2. This patch drops IncludeTagDefinition from CXPrintingPolicyProperty and does not attempt to replace it because TagSpecifierAs doesn't seem to fit that interface. This bit of functionality seems internal to the printing design, but I don't have any experience with CXPrintingPolicyProperty, so I might be missing an important use case. We could actually keep IncludeTagDefinition alongside TagSpecifierAs, but my naive guess is that IncludeTagDefinition is not reliable enough to be worthwhile. Perhaps Nikolai Kosjar or Jonathan Coe, who wrote/reviewed CXPrintingPolicyProperty can comment. https://reviews.llvm.org/D45463 Files: include/clang-c/Index.h include/clang/AST/PrettyPrinter.h lib/AST/DeclPrinter.cpp lib/AST/TypePrinter.cpp test/Misc/ast-print-enum-decl.c test/Misc/ast-print-record-decl.c tools/c-index-test/c-index-test.c tools/libclang/CIndex.cpp
Index: tools/libclang/CIndex.cpp =================================================================== --- tools/libclang/CIndex.cpp +++ tools/libclang/CIndex.cpp @@ -4739,8 +4739,6 @@ return P->SuppressSpecifiers; case CXPrintingPolicy_SuppressTagKeyword: return P->SuppressTagKeyword; - case CXPrintingPolicy_IncludeTagDefinition: - return P->IncludeTagDefinition; case CXPrintingPolicy_SuppressScope: return P->SuppressScope; case CXPrintingPolicy_SuppressUnwrittenScope: @@ -4808,9 +4806,6 @@ case CXPrintingPolicy_SuppressTagKeyword: P->SuppressTagKeyword = Value; return; - case CXPrintingPolicy_IncludeTagDefinition: - P->IncludeTagDefinition = Value; - return; case CXPrintingPolicy_SuppressScope: P->SuppressScope = Value; return; Index: tools/c-index-test/c-index-test.c =================================================================== --- tools/c-index-test/c-index-test.c +++ tools/c-index-test/c-index-test.c @@ -97,8 +97,6 @@ CXPrintingPolicy_SuppressSpecifiers}, {"CINDEXTEST_PRINTINGPOLICY_SUPPRESSTAGKEYWORD", CXPrintingPolicy_SuppressTagKeyword}, - {"CINDEXTEST_PRINTINGPOLICY_INCLUDETAGDEFINITION", - CXPrintingPolicy_IncludeTagDefinition}, {"CINDEXTEST_PRINTINGPOLICY_SUPPRESSSCOPE", CXPrintingPolicy_SuppressScope}, {"CINDEXTEST_PRINTINGPOLICY_SUPPRESSUNWRITTENSCOPE", Index: test/Misc/ast-print-record-decl.c =================================================================== --- /dev/null +++ test/Misc/ast-print-record-decl.c @@ -0,0 +1,251 @@ +// Check struct: +// +// First check compiling and printing of this file. +// +// RUN: %clang -Xclang -verify -S -emit-llvm -DKW=struct -DBASES= -o - %s \ +// RUN: | FileCheck --check-prefixes=CHECK,LLVM %s +// +// RUN: %clang_cc1 -verify -ast-print -DKW=struct -DBASES= %s > %t.c +// RUN: FileCheck --check-prefixes=CHECK,PRINT -DKW=struct -DBASES= %s \ +// RUN: --input-file %t.c +// +// Now check compiling and printing of the printed file. +// +// RUN: echo "// expected""-warning@* 10 {{'T' is deprecated}}" >> %t.c +// RUN: echo "// expected""-note@* 10 {{'T' has been explicitly marked deprecated here}}" >> %t.c +// +// RUN: %clang -Xclang -verify -S -emit-llvm -o - %t.c \ +// RUN: | FileCheck --check-prefixes=CHECK,LLVM %s +// +// RUN: %clang_cc1 -verify -ast-print %t.c \ +// RUN: | FileCheck --check-prefixes=CHECK,PRINT -DKW=struct -DBASES= %s + +// Repeat for union: +// +// First check compiling and printing of this file. +// +// RUN: %clang -Xclang -verify -S -emit-llvm -DKW=union -DBASES= -o - %s \ +// RUN: | FileCheck --check-prefixes=CHECK,LLVM %s +// +// RUN: %clang_cc1 -verify -ast-print -DKW=union -DBASES= %s > %t.c +// RUN: FileCheck --check-prefixes=CHECK,PRINT -DKW=union -DBASES= %s \ +// RUN: --input-file %t.c +// +// Now check compiling and printing of the printed file. +// +// RUN: echo "// expected""-warning@* 10 {{'T' is deprecated}}" >> %t.c +// RUN: echo "// expected""-note@* 10 {{'T' has been explicitly marked deprecated here}}" >> %t.c +// +// RUN: %clang -Xclang -verify -S -emit-llvm -o - %t.c \ +// RUN: | FileCheck --check-prefixes=CHECK,LLVM %s +// +// RUN: %clang_cc1 -verify -ast-print %t.c \ +// RUN: | FileCheck --check-prefixes=CHECK,PRINT -DKW=union -DBASES= %s + +// Repeat for C++ (BASES helps ensure we're printing as C++ not as C): +// +// First check compiling and printing of this file. +// +// RUN: %clang -Xclang -verify -S -emit-llvm -DKW=struct -DBASES=' : B' -o - \ +// RUN: -xc++ %s \ +// RUN: | FileCheck --check-prefixes=CHECK,LLVM %s +// +// RUN: %clang_cc1 -verify -ast-print -DKW=struct -DBASES=' : B' -xc++ %s \ +// RUN: > %t.cpp +// RUN: FileCheck --check-prefixes=CHECK,PRINT,CXX -DKW=struct \ +// RUN: -DBASES=' : B' %s --input-file %t.cpp +// +// Now check compiling and printing of the printed file. +// +// RUN: echo "// expected""-warning@* 10 {{'T' is deprecated}}" >> %t.cpp +// RUN: echo "// expected""-note@* 10 {{'T' has been explicitly marked deprecated here}}" >> %t.cpp +// +// RUN: %clang -Xclang -verify -S -emit-llvm -o - %t.cpp \ +// RUN: | FileCheck --check-prefixes=CHECK,LLVM %s +// +// RUN: %clang_cc1 -verify -ast-print %t.cpp \ +// RUN: | FileCheck --check-prefixes=CHECK,PRINT,CXX -DKW=struct \ +// RUN: -DBASES=' : B' %s + +// END. + +#ifndef KW +# error KW undefined +# define KW struct // help syntax checkers +#endif + +#ifndef BASES +# error BASES undefined +# define BASES // help syntax checkers +#endif + +struct B {}; + +// CHECK-LABEL: defFirst +void defFirst() { + // PRINT-NEXT: [[KW]] + // PRINT-DAG: __attribute__((aligned(16))) + // PRINT-DAG: __attribute__((deprecated(""))) + // PRINT-NOT: __attribute__ + // PRINT-SAME: T[[BASES]] { + // PRINT-NEXT: int i; + // PRINT-NEXT: } *p0; + // expected-warning@+2 {{'T' is deprecated}} + // expected-note@+1 2 {{'T' has been explicitly marked deprecated here}} + KW __attribute__((aligned(16))) __attribute__((deprecated(""))) T BASES { + int i; + } *p0; + + // PRINT-NEXT: [[KW]] T *p1; + KW T *p1; // expected-warning {{'T' is deprecated}} + + // LLVM: store i64 16 + long s0 = sizeof *p0; + // LLVM-NEXT: store i64 16 + long s1 = sizeof *p1; +} + +// CHECK-LABEL: defLast +void defLast() { + // PRINT-NEXT: [[KW]] __attribute__((aligned(16))) T *p0; + KW __attribute__((aligned(16))) T *p0; + + // PRINT-NEXT: [[KW]] + // PRINT-DAG: __attribute__((aligned(16))) + // PRINT-DAG: __attribute__((deprecated(""))) + // PRINT-NOT: __attribute__ + // PRINT-SAME: T[[BASES]] { + // PRINT-NEXT: int i; + // PRINT-NEXT: } *p1; + // expected-warning@+2 {{'T' is deprecated}} + // expected-note@+1 {{'T' has been explicitly marked deprecated here}} + KW __attribute__((deprecated(""))) T BASES { int i; } *p1; + + // LLVM: store i64 16 + long s0 = sizeof *p0; + // LLVM-NEXT: store i64 16 + long s1 = sizeof *p1; +} + +// CHECK-LABEL: defMiddle +void defMiddle() { + // PRINT-NEXT: [[KW]] __attribute__((deprecated(""))) T *p0; + // expected-warning@+2 {{'T' is deprecated}} + // expected-note@+1 3 {{'T' has been explicitly marked deprecated here}} + KW __attribute__((deprecated(""))) T *p0; + + // PRINT-NEXT: [[KW]] + // PRINT-DAG: __attribute__((deprecated(""))) + // PRINT-DAG: __attribute__((aligned(16))) + // PRINT-NOT: __attribute__ + // PRINT-SAME: T[[BASES]] { + // PRINT-NEXT: int i; + // PRINT-NEXT: } *p1; + KW __attribute__((aligned(16))) T BASES { int i; } *p1; // expected-warning {{'T' is deprecated}} + + // PRINT-NEXT: [[KW]] T *p2; + KW T *p2; // expected-warning {{'T' is deprecated}} + + // LLVM: store i64 16 + long s0 = sizeof *p0; + // LLVM-NEXT: store i64 16 + long s1 = sizeof *p1; + // LLVM-NEXT: store i64 16 + long s2 = sizeof *p2; +} + +// CHECK-LABEL: defSelfRef +void defSelfRef() { + // PRINT-NEXT: [[KW]] __attribute__((deprecated(""))) T *p0; + // expected-warning@+2 {{'T' is deprecated}} + // expected-note@+1 2 {{'T' has been explicitly marked deprecated here}} + KW __attribute__((deprecated(""))) T *p0; + + // PRINT-NEXT: [[KW]] + // PRINT-DAG: __attribute__((deprecated(""))) + // PRINT-DAG: __attribute__((aligned(16))) + // PRINT-NOT: __attribute__ + // PRINT-SAME: T[[BASES]] { + // PRINT-NEXT: int i; + // PRINT-NEXT: [[KW]] T *p2; + // PRINT-NEXT: } *p1; + KW __attribute__((aligned(16))) T BASES { // expected-warning {{'T' is deprecated}} + int i; + KW T *p2; + } *p1; + + // LLVM: store i64 16 + long s0 = sizeof *p0; + // LLVM-NEXT: store i64 16 + long s1 = sizeof *p1; + // LLVM-NEXT: store i64 16 + long s2 = sizeof *p0->p2; + // LLVM-NEXT: store i64 16 + long s3 = sizeof *p1->p2; + // LLVM-NEXT: store i64 16 + long s4 = sizeof *p1->p2->p2; +} + +// CHECK-LABEL: declsOnly +void declsOnly() { + // PRINT-NEXT: [[KW]] T *p0; + KW T *p0; + + // PRINT-NEXT: [[KW]] __attribute__((may_alias)) T *p1; + KW __attribute__((may_alias)) T *p1; + + // PRINT-NEXT: [[KW]] T *p2; + KW T *p2; + + // PRINT-NEXT: [[KW]] + // PRINT-DAG: __attribute__((may_alias)) + // PRINT-DAG: __attribute__((deprecated(""))) + // PRINT-NOT: __attribute__ + // PRINT-SAME: T *p3; + // expected-warning@+2 {{'T' is deprecated}} + // expected-note@+1 2 {{'T' has been explicitly marked deprecated here}} + KW __attribute__((deprecated(""))) T *p3; + + // PRINT-NEXT: [[KW]] T *p4; + KW T *p4; // expected-warning {{'T' is deprecated}} +} + +// Make sure expanded printing of tag types is turned back off in other parts +// of a tag declaration. The base class list is checked above. + +// CHECK-LABEL: inMembers +void inMembers() { + // PRINT-NEXT: [[KW]] T1 { + // PRINT-NEXT: int i; + // PRINT-NEXT: }; + KW T1 { int i; }; + // PRINT-NEXT: [[KW]] T2 { + // PRINT-NEXT: [[KW]] T1 i; + // PRINT-NEXT: }; + KW T2 { KW T1 i; }; +} + +// CHECK-LABEL: inInit +void inInit() { + // PRINT-NEXT: [[KW]] T1 { + // PRINT-NEXT: int i; + // PRINT-NEXT: }; + KW T1 { int i; }; + // PRINT-NEXT: [[KW]] T2 { + // PRINT-NEXT: long i; + // PRINT-NEXT: } t2 = {sizeof([[KW]] T1)}; + KW T2 { long i; } t2 = {sizeof(KW T1)}; +} + +#ifdef __cplusplus +// CXX-LABEL: inMemberPtr +void inMemberPtr() { + // CXX-NEXT: [[KW]] T1 { + // CXX-NEXT: int i; + // CXX-NEXT: }; + KW T1 { int i; }; + // CXX-NEXT: [[KW]] T2 { + // CXX-NEXT: } T1::*p; + KW T2 {} T1::*p; +} +#endif Index: test/Misc/ast-print-enum-decl.c =================================================================== --- /dev/null +++ test/Misc/ast-print-enum-decl.c @@ -0,0 +1,91 @@ +// First check compiling and printing of this file. +// +// RUN: %clang_cc1 -verify -ast-print %s > %t.c +// RUN: FileCheck --check-prefixes=CHECK,PRINT %s --input-file %t.c +// +// Now check compiling and printing of the printed file. +// +// RUN: echo "// expected""-warning@* 6 {{'T' is deprecated}}" >> %t.c +// RUN: echo "// expected""-note@* 6 {{'T' has been explicitly marked deprecated here}}" >> %t.c +// +// RUN: %clang_cc1 -verify -ast-print %t.c \ +// RUN: | FileCheck --check-prefixes=CHECK,PRINT %s + +// END. + +// CHECK-LABEL: defFirst +void defFirst() { + // PRINT-NEXT: enum + // PRINT-DAG: __attribute__((aligned(16))) + // PRINT-DAG: __attribute__((deprecated(""))) + // PRINT-SAME: T { + // PRINT-NEXT: E0, + // PRINT-NEXT: E1 + // PRINT-NEXT: } *p0; + // expected-warning@+2 {{'T' is deprecated}} + // expected-note@+1 2 {{'T' has been explicitly marked deprecated here}} + enum __attribute__((aligned(16))) __attribute__((deprecated(""))) T { + E0, E1 + } *p0; + + // PRINT-NEXT: enum T *p1; + enum T *p1; // expected-warning {{'T' is deprecated}} +} + +// CHECK-LABEL: defLast +void defLast() { + // PRINT-NEXT: enum __attribute__((aligned(16))) T *p0; + enum __attribute__((aligned(16))) T *p0; + + // PRINT-NEXT: enum + // PRINT-DAG: __attribute__((aligned(16))) + // PRINT-DAG: __attribute__((deprecated(""))) + // PRINT-SAME: T { + // PRINT-NEXT: E0, + // PRINT-NEXT: E1 + // PRINT-NEXT: } *p1; + // expected-warning@+2 {{'T' is deprecated}} + // expected-note@+1 {{'T' has been explicitly marked deprecated here}} + enum __attribute__((deprecated(""))) T { E0, E1 } *p1; +} + +// CHECK-LABEL: defMiddle +void defMiddle() { + // PRINT-NEXT: enum __attribute__((deprecated(""))) T *p0; + // expected-warning@+2 {{'T' is deprecated}} + // expected-note@+1 3 {{'T' has been explicitly marked deprecated here}} + enum __attribute__((deprecated(""))) T *p0; + + // PRINT-NEXT: enum + // PRINT-DAG: __attribute__((deprecated(""))) + // PRINT-DAG: __attribute__((aligned(16))) + // PRINT-SAME: T { + // PRINT-NEXT: E0 + // PRINT-NEXT: E1 + // PRINT-NEXT: } *p1; + enum __attribute__((aligned(16))) T { E0, E1 } *p1; // expected-warning {{'T' is deprecated}} + + // PRINT-NEXT: enum T *p2; + enum T *p2; // expected-warning {{'T' is deprecated}} +} + +// CHECK-LABEL: declsOnly +void declsOnly() { + // FIXME: For some reason, attributes are ignored if they're not on the first + // declaration and not on the definition. + + // PRINT-NEXT: enum __attribute__((aligned)) T *p0; + enum __attribute__((aligned)) T *p0; + + // PRINT-NEXT: enum T *p1; + enum __attribute__((may_alias)) T *p1; + + // PRINT-NEXT: enum T *p2; + enum T *p2; + + // PRINT-NEXT: enum T *p3; + enum __attribute__((deprecated(""))) T *p3; + + // PRINT-NEXT: enum T *p4; + enum T *p4; +} Index: lib/AST/TypePrinter.cpp =================================================================== --- lib/AST/TypePrinter.cpp +++ lib/AST/TypePrinter.cpp @@ -418,7 +418,7 @@ OS << '('; PrintingPolicy InnerPolicy(Policy); - InnerPolicy.IncludeTagDefinition = false; + InnerPolicy.TagSpecifierAs = nullptr; TypePrinter(InnerPolicy).print(QualType(T->getClass(), 0), OS, StringRef()); OS << "::*"; @@ -997,10 +997,12 @@ } void TypePrinter::printTag(TagDecl *D, raw_ostream &OS) { - if (Policy.IncludeTagDefinition) { + if (TagDecl *As = Policy.TagSpecifierAs) { + assert(D->getTypeForDecl() == As->getTypeForDecl() && + "tag declarations expected to be for the same type"); PrintingPolicy SubPolicy = Policy; - SubPolicy.IncludeTagDefinition = false; - D->print(OS, SubPolicy, Indentation); + SubPolicy.TagSpecifierAs = nullptr; + As->print(OS, SubPolicy, Indentation); spaceBeforePlaceHolder(OS); return; } @@ -1147,8 +1149,7 @@ void TypePrinter::printElaboratedBefore(const ElaboratedType *T, raw_ostream &OS) { // The tag definition will take care of these. - if (!Policy.IncludeTagDefinition) - { + if (!Policy.TagSpecifierAs) { OS << TypeWithKeyword::getKeywordName(T->getKeyword()); if (T->getKeyword() != ETK_None) OS << " "; Index: lib/AST/DeclPrinter.cpp =================================================================== --- lib/AST/DeclPrinter.cpp +++ lib/AST/DeclPrinter.cpp @@ -177,13 +177,13 @@ bool isFirst = true; for ( ; Begin != End; ++Begin) { if (isFirst) { - if(TD) - SubPolicy.IncludeTagDefinition = true; + if (TD) + SubPolicy.TagSpecifierAs = TD; SubPolicy.SuppressSpecifiers = false; isFirst = false; } else { if (!isFirst) Out << ", "; - SubPolicy.IncludeTagDefinition = false; + SubPolicy.TagSpecifierAs = nullptr; SubPolicy.SuppressSpecifiers = true; } @@ -845,7 +845,7 @@ } PrintingPolicy SubPolicy(Policy); SubPolicy.SuppressSpecifiers = false; - SubPolicy.IncludeTagDefinition = false; + SubPolicy.TagSpecifierAs = nullptr; Init->printPretty(Out, nullptr, SubPolicy, Indentation); if ((D->getInitStyle() == VarDecl::CallInit) && !isa<ParenListExpr>(Init)) Out << ")"; Index: include/clang/AST/PrettyPrinter.h =================================================================== --- include/clang/AST/PrettyPrinter.h +++ include/clang/AST/PrettyPrinter.h @@ -39,8 +39,7 @@ /// \brief Create a default printing policy for the specified language. PrintingPolicy(const LangOptions &LO) : Indentation(2), SuppressSpecifiers(false), - SuppressTagKeyword(LO.CPlusPlus), - IncludeTagDefinition(false), SuppressScope(false), + SuppressTagKeyword(LO.CPlusPlus), SuppressScope(false), SuppressUnwrittenScope(false), SuppressInitializers(false), ConstantArraySizeAsWritten(false), AnonymousTagLocations(true), SuppressStrongLifetime(false), SuppressLifetimeQualifiers(false), @@ -52,7 +51,7 @@ Half(LO.Half), MSWChar(LO.MicrosoftExt && !LO.WChar), IncludeNewlines(true), MSVCFormatting(false), ConstantsAsWritten(false), SuppressImplicitBase(false), - FullyQualifiedName(false) { } + FullyQualifiedName(false), TagSpecifierAs(nullptr) { } /// Adjust this printing policy for cases where it's known that we're /// printing C++ code (for instance, if AST dumping reaches a C++-only @@ -93,16 +92,6 @@ /// \endcode bool SuppressTagKeyword : 1; - /// When true, include the body of a tag definition. - /// - /// This is used to place the definition of a struct - /// in the middle of another declaration as with: - /// - /// \code - /// typedef struct { int x, y; } Point; - /// \endcode - bool IncludeTagDefinition : 1; - /// Suppresses printing of scope specifiers. bool SuppressScope : 1; @@ -225,6 +214,31 @@ /// When true, print the fully qualified name of function declarations. /// This is the opposite of SuppressScope and thus overrules it. bool FullyQualifiedName : 1; + + /// When not nullptr, print the tag specifier in the immediate specifier list + /// using this tag declaration, but do not do so again for nested + /// occurrences. + /// + /// \c TagSpecifierAs is designed specifically to be used by + /// \c Decl::printGroup to print tag specifiers as recorded in the AST so + /// that the printed source compiles successfully if the original source did. + /// \c TagSpecifierAs does not easily fit the \c CXPrintingPolicyProperty + /// interface and so currently is not exposed there. + /// + /// FIXME: The TagSpecifierAs mechanism does not always print source that is + /// exactly faithful to the original source because, when building the AST, + /// the parser (1) adds all attributes declared on a tag to all later + /// redeclarations of that tag (that is, tag occurrences declaring an + /// attribute or member list), and (2) drops attributes declared on a tag + /// after an occurrence of that tag declaring a member list. The first + /// change should not affect semantics or diagnostics because it merely + /// reveals the accumulated attributes. The second change only loses + /// warnings about how an attribute cannot be declared after the member list + /// has been specified, and such attributes otherwise should have no effect. + /// Thus, fixing these changes doesn't seem vital. Moreover, fixing them + /// would require the parser to build the AST with an exact record of which + /// attributes are introduced at each tag (re)declaration. + TagDecl *TagSpecifierAs; }; } // end namespace clang Index: include/clang-c/Index.h =================================================================== --- include/clang-c/Index.h +++ include/clang-c/Index.h @@ -4113,7 +4113,6 @@ CXPrintingPolicy_Indentation, CXPrintingPolicy_SuppressSpecifiers, CXPrintingPolicy_SuppressTagKeyword, - CXPrintingPolicy_IncludeTagDefinition, CXPrintingPolicy_SuppressScope, CXPrintingPolicy_SuppressUnwrittenScope, CXPrintingPolicy_SuppressInitializers,
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits