[PATCH] D46919: [libclang] Deprecate CXPrintingPolicy_IncludeTagDefinition

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

In https://reviews.llvm.org/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.



================
Comment at: include/clang/AST/PrettyPrinter.h:100-108
+  /// When true, print any tag declaration owned by an elaborated type.
   ///
-  /// This is used to place the definition of a struct
-  /// in the middle of another declaration as with:
+  /// 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;
----------------
rsmith wrote:
> 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.
> 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?


https://reviews.llvm.org/D46919