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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
