[AST] Print correct tag decl for tag specifier

[jdenny]

Summary

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.

Given:

void fn() {
  struct T *p0;
  struct __attribute__((deprecated)) T *p1;
}

-ast-print dropped the attribute.

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. Redefinition
errors were sometimes introduced, as in the first example above. Even
when that was impossible because no member list was ever specified,
attributes were sometimes lost, thus changing semantics and
diagnostics, as in the second example above.

This patch fixes a major culprit for these problems. It does so by
creating an ElaboratedType with a new OwnedDecl member wherever an
occurrence of a tag type is a (re)declaration of that tag type.
PrintingPolicy's IncludeTagDefinition used to trigger printing of the
member list, attributes, etc. for a tag specifier by using a tag
(re)declaration selected as described above. Now, it triggers the
same thing except it uses the tag (re)declaration stored in the
OwnedDecl. Of course, other tooling can now make use of the new
OwnedDecl as well.

Also, to be more faithful to the original source, this patch
suppresses printing of attributes inherited from previous
declarations.

[jdenny]

Rebased onto a more recent master.

[jdenny]

Rebased. Ping.

[rsmith]

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.

This seems fine. See r270009 for some background for that comment -- we used to store a copy of the LangOptions in the PrintingPolicy (among other things), and copying these objects (including the potentially-large vectors within the LangOptions) was a double-digit percentage of the compile time of some compilations. That's a very different ball park from a change from 8 bytes to 16 bytes.

include/clang-c/Index.h

4116

This is not OK: this would be an ABI break for the stable libclang ABI.

lib/AST/DeclPrinter.cpp

180–182

Instead of the changes in this patch, can you address your use case by just changing the if here to if (TD && TD->isThisDeclarationADefinition())?

[jdenny]

In D45463#1087174, @rsmith wrote:

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.

This seems fine. See r270009 for some background for that comment -- we used to store a copy of the LangOptions in the PrintingPolicy (among other things), and copying these objects (including the potentially-large vectors within the LangOptions) was a double-digit percentage of the compile time of some compilations. That's a very different ball park from a change from 8 bytes to 16 bytes.

Makes sense. Thanks.

include/clang-c/Index.h

4116

Understood. We could keep IncludeTagDefinition for that purpose and just replace its current internal use with TagSpecifierAs.

Out of curiosity, do we know how others are using IncludeTagDefinition?

lib/AST/DeclPrinter.cpp

180–182

It sounds like that would fix the case of duplicated definitions.

It doesn't address the case of dropped or relocated attributes. For example, -ast-print drops the following attribute and thus the deprecation warning:

void fn() {
  struct T *p0;
  struct __attribute__((deprecated)) T *p1;
}

I don't know how clang *should* accept and interpret attributes in such cases. However, it seems wrong for -ast-print to produce a program that clang interprets differently than the original program.

[aaron.ballman]

lib/AST/DeclPrinter.cpp

180–182

I don't know how clang *should* accept and interpret attributes in such cases. However, it seems wrong for -ast-print to produce a program that clang interprets differently than the original program.

This has been a *very* long-standing issue with -ast-print that I'd love to see fixed. The AST printer does not print attributes in the correct locations except by accident, and it oftentimes prints with the wrong syntax, or is missing from places where it should be printed.

Long story short, I agree that this behavior is wrong and needs to be fixed, but it's also going to take a bit of work to get it right.

[rsmith]

lib/AST/DeclPrinter.cpp

180–182

it seems wrong for -ast-print to produce a program that clang interprets differently than the original program.

Historically we've viewed -ast-print as a debugging tool, not as a way of producing valid source code. If you want to change that, there will be a loooong list of issues to fix. However, one of the important and intended uses of the -ast-print mechanism is producing pretty-printed type names and the like for diagnostics, and in that context, it *is* important to print attributes properly.

It doesn't address the case of dropped or relocated attributes.

Hmm. A big part of the problem here, in my view, is that when we come to print a TagType, we call TagType::getDecl, which returns our "favourite" declaration of that entity, not necessarily the one that was named (or even created) for that instance of the type. We could easily fix that (by adding an accessor to reach TagType::decl rather than calling TagType::getDecl()), and perhaps *that* plus the current tracking of whether we're printing the tag declaration that is owned by the current DeclGroup would be enough to get correct displays here.

Another part of the problem is that TagType does not track whether it's referring to its "own" TagDecl (ie, one created when parsing that type) or some pre-existing one, and that affects whether and how attributes should be printed. That's why we need the IncludeTagDefinition flag in the first place; it's our attempt to recreate knowledge about whether the TagType owns its TagDecl. We could consider instead tracking a flag on TagType sugar nodes to model that, which would let us get rid of that flag.

[rsmith]

lib/AST/DeclPrinter.cpp

180–182

Hmm. A big part of the problem here, in my view, is that when we come to print a TagType, we call TagType::getDecl, which returns our "favourite" declaration of that entity, not necessarily the one that was named (or even created) for that instance of the type.

Ah, no, the problem is a little deeper than that; we don't even create distinct type sugar for the new-decl vs existing-decl cases. Maybe that'd be somewhere to start -- perhaps the ElaboratedType node could track that for us.

[jdenny]

lib/AST/DeclPrinter.cpp

180–182

Historically we've viewed -ast-print as a debugging tool, not as a way of producing valid source code. If you want to change that, there will be a loooong list of issues to fix.

I've heard that, but it seemed to me that what's good for the latter is likely good for the former.

Still, I don't want to waste time on patches that won't be accepted because they're harder to justify purely for debugging purposes. Is there a better place to explore pretty printing valid source code from a clang AST?

However, one of the important and intended uses of the -ast-print mechanism is producing pretty-printed type names and the like for diagnostics, and in that context, it *is* important to print attributes properly.

I haven't considered cases where diagnostics are broken by this issue. For diagnostics, is it important to print the exact set of attributes on a particular declaration rather than to print, say, the entire list of attributes from all declarations of the tag? The latter appears to be easier to implement.

Ah, no, the problem is a little deeper than that; we don't even create distinct type sugar for the new-decl vs existing-decl cases. Maybe that'd be somewhere to start -- perhaps the ElaboratedType node could track that for us.

My approach was to constrain my changes to the -ast-print implementation as much as possible without changing the AST in order to avoid breaking something else. I found that I could fix a big problem (printing the right tag decl at each specifier) just using the AST as it's already constructed. DeclGroup gives us the info we need. That info just isn't in the most convenient place, as I think you're pointing out. (Also keep in mind that there's a child patch that fixes decl group discovery in some contexts not exercised by this patch.)

If you feel that ElaboratedType is a better way to go, I'll look into it and get back to you later.

[jdenny]

I've implemented the suggestion to use ElaboratedType. See the last paragraph of the revised summary for details.

There might be trouble for CXPrintingPolicyProperty users. That is, this patch shifts the semantics of IncludeTagDefinition away from the documented semantics and from what the name implies. Then again, both were already misleading for a tag type without a member list (that is, definition).

I see a few possible paths:

1. Update the documentation for IncludeTagDefinition and assume the new behavior was the expected behavior all along.

2. #1 and rename IncludeTagDefinition to something like OwnedTagDeclaration.

3. Add OwnedTagDeclaration alongside IncludeTagDefinition. CXPrintingPolicyProperty users could set either. We'd have to decide which would have precedence. Internally, we would change Decl::printGroup to set OwnedTagDeclaration instead of IncludeTagDefinition, which otherwise would not be used internally.

[rsmith]

include/clang/AST/Type.h

4852–4855 ↗

(On Diff #146145)

I think it's better to keep this as-is: the "owns a (re)declaration" case is a special case of using an elaborated type keyword.

4905 ↗

(On Diff #146145)

Are there other types for which it would be meaningful? (If not, we could store and expose this as a TagDecl instead.)

4906–4916 ↗

(On Diff #146145)

You can use isInherited() on the attribute to find out whether it was written on that declaration or inherited.

[jdenny]

Made the suggested changes. Thanks.

[jdenny]

lib/AST/DeclPrinter.cpp

218

I implemented inherited attribute suppression in this function with the expectation that the test suite would reveal a use case where that's wrong. However, the test suite behaved. If you know of a use case, please let me know.

[rsmith]

LGTM, thanks!

include/clang/AST/Type.h

4903–4910 ↗

(On Diff #146351)

Please remove this FIXME. This function *is* returning the right TagDecl. This is the wrong place to call out any infelicities in attribute retention.

[jdenny]

In D45463#1096406, @rsmith wrote:

LGTM, thanks!

Thanks. I'll remove the fixme as you asked.

A few comments ago, I mentioned that IncludeTagDefinition's documentation and name is drifting farther from its functionality. Should we do something about that?

[rsmith]

In D45463#1096499, @jdenny wrote:

A few comments ago, I mentioned that IncludeTagDefinition's documentation and name is drifting farther from its functionality. Should we do something about that?

I think we should deprecate that flag, and change it to have no effect. It is an implementation detail of the AST printer and should never have been exposed by libclang in the first place.

[jdenny]

In D45463#1096629, @rsmith wrote:

In D45463#1096499, @jdenny wrote:

A few comments ago, I mentioned that IncludeTagDefinition's documentation and name is drifting farther from its functionality. Should we do something about that?

I think we should deprecate that flag, and change it to have no effect. It is an implementation detail of the AST printer and should never have been exposed by libclang in the first place.

Thanks. I'll address that in a separate patch then.

include/clang-c/Index.h

  * \brief Properties for the printing policy.
  *
  * See \c clang::PrintingPolicy for more information.
  */
 enum CXPrintingPolicyProperty {
   CXPrintingPolicy_Indentation,
   CXPrintingPolicy_SuppressSpecifiers,
   CXPrintingPolicy_SuppressTagKeyword,
-  CXPrintingPolicy_IncludeTagDefinition,
   CXPrintingPolicy_SuppressScope,
   CXPrintingPolicy_SuppressUnwrittenScope,
   CXPrintingPolicy_SuppressInitializers,
   CXPrintingPolicy_ConstantArraySizeAsWritten,
   CXPrintingPolicy_AnonymousTagLocations,
   CXPrintingPolicy_SuppressStrongLifetime,
   CXPrintingPolicy_SuppressLifetimeQualifiers,
   CXPrintingPolicy_SuppressTemplateArgsInCXXConstructors,

include/clang/AST/PrettyPrinter.h

 /// printed.
 ///
 /// This type is intended to be small and suitable for passing by value.
 /// It is very frequently copied.
 struct PrintingPolicy {
   /// \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),
       SuppressTemplateArgsInCXXConstructors(false),
       Bool(LO.Bool), Restrict(LO.C99),
       Alignof(LO.CPlusPlus11), UnderscoreAlignof(LO.C11),
       UseVoidForZeroParams(!LO.CPlusPlus),
       TerseOutput(false), PolishForDeclaration(false),
       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
   /// construct). This should not be used if a real LangOptions object is
   /// available.
   void adjustForCPlusPlus() {
     SuppressTagKeyword = true;
     Bool = true;
struct PrintingPolicy {
   /// This is used when printing the inner type of elaborated types,
   /// (as the tag keyword is part of the elaborated type):
   ///
   /// \code
   /// struct Geometry::Point;
   /// \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;
 
   /// Suppress printing parts of scope specifiers that don't need
   /// to be written, e.g., for inline or anonymous namespaces.
   bool SuppressUnwrittenScope : 1;
   
   /// Suppress printing of variable initializers.
struct PrintingPolicy {
   bool ConstantsAsWritten : 1;
 
   /// When true, don't print the implicit 'self' or 'this' expressions.
   bool SuppressImplicitBase : 1;
 
   /// 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
 
 #endif

lib/AST/DeclPrinter.cpp

void Decl::printGroup(Decl** Begin, unsigned NumDecls,
   if (TD)
     ++Begin;
 
   PrintingPolicy SubPolicy(Policy);
 
   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;
     }
 
     (*Begin)->print(Out, SubPolicy, Indentation);
   }
 }
 
 LLVM_DUMP_METHOD void DeclContext::dumpDeclContext() const {

 
 void DeclPrinter::prettyPrintAttributes(Decl *D) {
   if (Policy.PolishForDeclaration)
     return;
 
   if (D->hasAttrs()) {
     AttrVec &Attrs = D->getAttrs();
     for (auto *A : Attrs) {
       switch (A->getKind()) {
 #define ATTR(X)
 #define PRAGMA_SPELLING_ATTR(X) case attr::X:
 #include "clang/Basic/AttrList.inc"
         break;
       default:
         A->printPretty(Out, Policy);
         break;
       }
if (!Policy.SuppressInitializers && Init) {
     if (!ImplicitInit) {
       if ((D->getInitStyle() == VarDecl::CallInit) && !isa<ParenListExpr>(Init))
         Out << "(";
       else if (D->getInitStyle() == VarDecl::CInit) {
         Out << " = ";
       }
       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 << ")";
     }
   }
   prettyPrintAttributes(D);
 }
 

lib/AST/TypePrinter.cpp

void TypePrinter::printMemberPointerBefore(const MemberPointerType *T,
   SaveAndRestore<bool> NonEmptyPH(HasEmptyPlaceHolder, false);
   printBefore(T->getPointeeType(), OS);
   // Handle things like 'int (Cls::*A)[4];' correctly.
   // FIXME: this should include vectors, but vectors use attributes I guess.
   if (isa<ArrayType>(T->getPointeeType()))
     OS << '(';
 
   PrintingPolicy InnerPolicy(Policy);
-  InnerPolicy.IncludeTagDefinition = false;
+  InnerPolicy.TagSpecifierAs = nullptr;
   TypePrinter(InnerPolicy).print(QualType(T->getClass(), 0), OS, StringRef());
 
   OS << "::*";
 }
 
 void TypePrinter::printMemberPointerAfter(const MemberPointerType *T, 
                                           raw_ostream &OS) { 
   IncludeStrongLifetimeRAII Strong(Policy);
if (const auto *NS = dyn_cast<NamespaceDecl>(DC)) {
     else if (Tag->getIdentifier())
       OS << Tag->getIdentifier()->getName() << "::";
     else
       return;
   }
 }
 
 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;
   }
 
   bool HasKindDecoration = false;
 
   // We don't print tags unless this is an elaborated type.
   // In C, we just assume every RecordType is an elaborated type.

 }
 
 void TypePrinter::printInjectedClassNameAfter(const InjectedClassNameType *T,
                                                raw_ostream &OS) {}
 
 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 << " ";
     NestedNameSpecifier *Qualifier = T->getQualifier();
     if (Qualifier)
       Qualifier->print(OS, Policy);
   }
   

test/Misc/ast-print-enum-decl.c

+// 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;
+}

test/Misc/ast-print-record-decl.c

+// 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

tools/c-index-test/c-index-test.c

struct Mapping {
     enum CXPrintingPolicyProperty property;
   };
   struct Mapping mappings[] = {
       {"CINDEXTEST_PRINTINGPOLICY_INDENTATION", CXPrintingPolicy_Indentation},
       {"CINDEXTEST_PRINTINGPOLICY_SUPPRESSSPECIFIERS",
        CXPrintingPolicy_SuppressSpecifiers},
       {"CINDEXTEST_PRINTINGPOLICY_SUPPRESSTAGKEYWORD",
        CXPrintingPolicy_SuppressTagKeyword},
-      {"CINDEXTEST_PRINTINGPOLICY_INCLUDETAGDEFINITION",
-       CXPrintingPolicy_IncludeTagDefinition},
       {"CINDEXTEST_PRINTINGPOLICY_SUPPRESSSCOPE",
        CXPrintingPolicy_SuppressScope},
       {"CINDEXTEST_PRINTINGPOLICY_SUPPRESSUNWRITTENSCOPE",
        CXPrintingPolicy_SuppressUnwrittenScope},
       {"CINDEXTEST_PRINTINGPOLICY_SUPPRESSINITIALIZERS",
        CXPrintingPolicy_SuppressInitializers},
       {"CINDEXTEST_PRINTINGPOLICY_CONSTANTARRAYSIZEASWRITTEN",
        CXPrintingPolicy_ConstantArraySizeAsWritten},

tools/libclang/CIndex.cpp

clang_PrintingPolicy_getProperty(CXPrintingPolicy Policy,
   PrintingPolicy *P = static_cast<PrintingPolicy *>(Policy);
   switch (Property) {
   case CXPrintingPolicy_Indentation:
     return P->Indentation;
   case CXPrintingPolicy_SuppressSpecifiers:
     return P->SuppressSpecifiers;
   case CXPrintingPolicy_SuppressTagKeyword:
     return P->SuppressTagKeyword;
-  case CXPrintingPolicy_IncludeTagDefinition:
-    return P->IncludeTagDefinition;
   case CXPrintingPolicy_SuppressScope:
     return P->SuppressScope;
   case CXPrintingPolicy_SuppressUnwrittenScope:
     return P->SuppressUnwrittenScope;
   case CXPrintingPolicy_SuppressInitializers:
     return P->SuppressInitializers;
   case CXPrintingPolicy_ConstantArraySizeAsWritten:
     return P->ConstantArraySizeAsWritten;
case CXPrintingPolicy_Indentation:
     P->Indentation = Value;
     return;
   case CXPrintingPolicy_SuppressSpecifiers:
     P->SuppressSpecifiers = Value;
     return;
   case CXPrintingPolicy_SuppressTagKeyword:
     P->SuppressTagKeyword = Value;
     return;
-  case CXPrintingPolicy_IncludeTagDefinition:
-    P->IncludeTagDefinition = Value;
-    return;
   case CXPrintingPolicy_SuppressScope:
     P->SuppressScope = Value;
     return;
   case CXPrintingPolicy_SuppressUnwrittenScope:
     P->SuppressUnwrittenScope = Value;
     return;
   case CXPrintingPolicy_SuppressInitializers:
     P->SuppressInitializers = Value;