[libclang] Deprecate CXPrintingPolicy_IncludeTagDefinition

[jdenny]

Summary

And make it have no effect. Suggested at:

https://reviews.llvm.org/D45463#1096629

[rsmith]

The deprecated enumerator is also referenced by tools/c-index-test/c-index-test.c

This looks fine to me, but let's leave this review thread open for a week or so, to give people a bit more time to object in case they're using the flag for something.

include/clang/AST/PrettyPrinter.h

97–99 ↗

(On Diff #146964)

There's no need to keep this around. We have no API stability guarantees for our C++ API.

100–108 ↗

(On Diff #146964)

Please explicitly mark this as being transient state that's internal to the printing algorithm and not part of its external configuration, so that people don't try to expose it again in the future. The same is true for a bunch of the other flags around here -- if we're going to do this, we should systematically examine these flags to see which ones are external configuration and which ones are internal state that we're (hackily) passing between printing steps via the PrintingPolicy. It would also make sense to move the internal state flags to a separate struct from the policy.

[jdenny]

In D46919#1100493, @rsmith wrote:

The deprecated enumerator is also referenced by tools/c-index-test/c-index-test.c

This test prompted me to keep the IncludeTagDefinition member in PrintingPolicy so that clang_PrintingPolicy_getProperty would return the previous value set by clang_PrintingPolicy_setProperty. Otherwise, the value doesn't have any effect. Is that self-consistency not worth worrying about? If so, I'll remove both.

This looks fine to me, but let's leave this review thread open for a week or so, to give people a bit more time to object in case they're using the flag for something.

Sure.

include/clang/AST/PrettyPrinter.h

100–108 ↗

(On Diff #146964)

Please explicitly mark this as being transient state that's internal to the printing algorithm and not part of its external configuration, so that people don't try to expose it again in the future.

Will do.

The same is true for a bunch of the other flags around here -- if we're going to do this, we should systematically examine these flags to see which ones are external configuration and which ones are internal state that we're (hackily) passing between printing steps via the PrintingPolicy.

Doing that now sounds like a lot of analysis and difficult judgment calls without much immediate gain -- at least for me as I'm not so familiar with all the flags and their potential uses from libclang. My inclination is to write a fixme and refactor gradually.

It would also make sense to move the internal state flags to a separate struct from the policy.

Are you ok with making the new struct a member of the existing policy to simplify the refactoring effort?

[jdenny]

Made a stab at the suggested changes.

[jdenny]

In D46919#1101268, @jdenny wrote:

In D46919#1100493, @rsmith wrote:

The deprecated enumerator is also referenced by tools/c-index-test/c-index-test.c

This test prompted me to keep the IncludeTagDefinition member in PrintingPolicy so that clang_PrintingPolicy_getProperty would return the previous value set by clang_PrintingPolicy_setProperty. Otherwise, the value doesn't have any effect. Is that self-consistency not worth worrying about? If so, I'll remove both.

Sorry, I was thinking of a different test. I've removed the reference you mentioned.

include/clang/AST/PrettyPrinter.h

97–99 ↗

(On Diff #146964)

The test LibclangPrintingPolicyTest in unittests/libclang/LibclangTest.cpp wants clang_PrintingPolicy_getProperty to return the previous value set by clang_PrintingPolicy_setProperty for every member of CXPrintingPolicyProperty. Keeping this field makes that possible.

[rsmith]

In D46919#1101268, @jdenny wrote:

In D46919#1100493, @rsmith wrote:

The deprecated enumerator is also referenced by tools/c-index-test/c-index-test.c

This test prompted me to keep the IncludeTagDefinition member in PrintingPolicy so that clang_PrintingPolicy_getProperty would return the previous value set by clang_PrintingPolicy_setProperty. Otherwise, the value doesn't have any effect. Is that self-consistency not worth worrying about? If so, I'll remove both.

I don't think it's worth worrying about. We don't guarantee that the values round-trip in general (most of them are unsigneds being written to a bool, so we don't preserve values that are neither 0 nor 1).

[jdenny]

In D46919#1107296, @rsmith wrote:

In D46919#1101268, @jdenny wrote:

In D46919#1100493, @rsmith wrote:

The deprecated enumerator is also referenced by tools/c-index-test/c-index-test.c

This test prompted me to keep the IncludeTagDefinition member in PrintingPolicy so that clang_PrintingPolicy_getProperty would return the previous value set by clang_PrintingPolicy_setProperty. Otherwise, the value doesn't have any effect. Is that self-consistency not worth worrying about? If so, I'll remove both.

I don't think it's worth worrying about. We don't guarantee that the values round-trip in general (most of them are unsigneds being written to a bool, so we don't preserve values that are neither 0 nor 1).

I was actually thinking of a different test (see my comments from today), and it wants 0 and 1 to be preserved.

[jdenny]

Rebased. Ping.

[jdenny]

Ping. Rebased.

[jdenny]

Ping. Rebased.

[jdenny]

Ping. Rebased. This patch intends to perform cleanup that @rsmith suggested while reviewing another patch from me. If there's no more interest, it's fine with me to abandon. I'll wait at least one more week.

clang/include/clang-c/Index.h

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

clang/include/clang/AST/PrettyPrinter.h

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
   unsigned 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
+  /// This flag is deprecated and no longer has any effect.
   unsigned IncludeTagDefinition : 1;
 
   /// Suppresses printing of scope specifiers.
   unsigned SuppressScope : 1;
 
   /// Suppress printing parts of scope specifiers that don't need
   /// to be written, e.g., for inline or anonymous namespaces.
   unsigned SuppressUnwrittenScope : 1;
struct PrintingPolicy {
   unsigned ConstantsAsWritten : 1;
 
   /// When true, don't print the implicit 'self' or 'this' expressions.
   unsigned SuppressImplicitBase : 1;
 
   /// When true, print the fully qualified name of function declarations.
   /// This is the opposite of SuppressScope and thus overrules it.
   unsigned FullyQualifiedName : 1;
+
+  /// Transient state that's internal to the printing algorithm and that's not
+  /// intended to be exposed as part of its external configuration (via
+  /// libclang, for example).
+  ///
+  /// FIXME: Many more flags above should be moved here and should be
+  /// deprecated in libclang's CXPrintingPolicyProperty.
+  struct TransientState {
+    TransientState() : PrintOwnedTagDecl(false) {}
+    /// When true, print any tag declaration owned by an elaborated type.
+    ///
+    /// This is used to faithfully print the exact tag declaration that appears
+    /// within another declaration.  For example:
+    ///
+    /// \code
+    /// typedef struct T { int x, y; } Point;
+    /// \endcode
+    unsigned PrintOwnedTagDecl : 1;
+  } State;
 };
 
 } // end namespace clang
 
 #endif

clang/lib/AST/DeclPrinter.cpp

if (TD)
     ++Begin;
 
   PrintingPolicy SubPolicy(Policy);
 
   bool isFirst = true;
   for ( ; Begin != End; ++Begin) {
     if (isFirst) {
       if(TD)
-        SubPolicy.IncludeTagDefinition = true;
+        SubPolicy.State.PrintOwnedTagDecl = true;
       SubPolicy.SuppressSpecifiers = false;
       isFirst = false;
     } else {
       if (!isFirst) Out << ", ";
-      SubPolicy.IncludeTagDefinition = false;
+      SubPolicy.State.PrintOwnedTagDecl = false;
       SubPolicy.SuppressSpecifiers = true;
     }
 
     (*Begin)->print(Out, SubPolicy, Indentation);
   }
 }
 
 LLVM_DUMP_METHOD void DeclContext::dumpDeclContext() const {
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.State.PrintOwnedTagDecl = false;
       Init->printPretty(Out, nullptr, SubPolicy, Indentation);
       if ((D->getInitStyle() == VarDecl::CallInit) && !isa<ParenListExpr>(Init))
         Out << ")";
     }
   }
   prettyPrintAttributes(D);
 }
 

clang/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.State.PrintOwnedTagDecl = false;
   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 (Policy.State.PrintOwnedTagDecl) {
     PrintingPolicy SubPolicy = Policy;
-    SubPolicy.IncludeTagDefinition = false;
+    SubPolicy.State.PrintOwnedTagDecl = false;
     D->print(OS, SubPolicy, Indentation);
     spaceBeforePlaceHolder(OS);
     return;
   }
 
   bool HasKindDecoration = false;
 
   // We don't print tags unless this is an elaborated type.
void TypePrinter::printInjectedClassNameBefore(const InjectedClassNameType *T,
   printTemplateSpecializationBefore(T->getInjectedTST(), OS);
 }
 
 void TypePrinter::printInjectedClassNameAfter(const InjectedClassNameType *T,
                                                raw_ostream &OS) {}
 
 void TypePrinter::printElaboratedBefore(const ElaboratedType *T,
                                         raw_ostream &OS) {
-  if (Policy.IncludeTagDefinition && T->getOwnedTagDecl()) {
+  if (Policy.State.PrintOwnedTagDecl && T->getOwnedTagDecl()) {
     TagDecl *OwnedTagDecl = T->getOwnedTagDecl();
     assert(OwnedTagDecl->getTypeForDecl() == T->getNamedType().getTypePtr() &&
            "OwnedTagDecl expected to be a declaration for the type");
     PrintingPolicy SubPolicy = Policy;
-    SubPolicy.IncludeTagDefinition = false;
+    SubPolicy.State.PrintOwnedTagDecl = false;
     OwnedTagDecl->print(OS, SubPolicy, Indentation);
     spaceBeforePlaceHolder(OS);
     return;
   }
 
   // The tag definition will take care of these.
-  if (!Policy.IncludeTagDefinition)
-  {
+  if (!Policy.State.PrintOwnedTagDecl) {
     OS << TypeWithKeyword::getKeywordName(T->getKeyword());
     if (T->getKeyword() != ETK_None)
       OS << " ";
     NestedNameSpecifier *Qualifier = T->getQualifier();
     if (Qualifier)
       Qualifier->print(OS, Policy);
   }
 
   ElaboratedTypePolicyRAII PolicyRAII(Policy);
   printBefore(T->getNamedType(), OS);
 }
 
 void TypePrinter::printElaboratedAfter(const ElaboratedType *T,
                                         raw_ostream &OS) {
-  if (Policy.IncludeTagDefinition && T->getOwnedTagDecl())
+  if (Policy.State.PrintOwnedTagDecl && T->getOwnedTagDecl())
     return;
   ElaboratedTypePolicyRAII PolicyRAII(Policy);
   printAfter(T->getNamedType(), OS);
 }
 
 void TypePrinter::printParenBefore(const ParenType *T, raw_ostream &OS) {
   if (!HasEmptyPlaceHolder && !isa<FunctionType>(T->getInnerType())) {
     printBefore(T->getInnerType(), OS);

clang/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},