[PATCH] D45093: [AST] Fix -ast-print for _Bool when have diagnostics

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

jdenny added a comment.

In https://reviews.llvm.org/D45093#1090292, @rsmith wrote:

> If you want to force a particular printing policy to be used for `-ast-print`, I think it would be better to change the `print` call in `lib/Frontend/ASTConsumers.cpp` to pass your desired printing policy, rather than changing other components to prevent them from changing the `ASTContext`'s default printing policy.


Thanks.  I'll look into that.

> (For what it's worth, I think we should also try to change the way we print diagnostics so that `Sema` is asked for a printing policy when it's needed rather than it setting global `ASTContext` state each time we consider printing a diagnostic. That way we could also take the `SourceLocation` information into account to figure out whether `bool` is suitably `#define`d at the point of the diagnostic rather than checking whether it's defined at the current end-of-preprocessing state. But that's not really relevant for this patch, except that generally I think we should be moving away from stashing a global `PrintingPolicy` on the `ASTContext`.)

I'll keep that in mind in case I have more work in that area.

>>> I'd like @rsmith's opinion on whether we should be trying to make -ast-print have good source fidelity or not. I was under the impression we wanted -ast-print to faithfully reproduce code at least as a low priority desire, but it sounds like it may only be intended as an approximation of the user's source code, so adding extra machinery to support better fidelity may be more maintenance burden than it's worth.
>> 
>> Given the discussion in https://reviews.llvm.org/D45463, it seems I need a more precise understanding of the purpose of -ast-print before I write any more fixes like these.
> 
> As things stand, I'd generally consider `-ast-print` to be a "best-effort" feature: we don't guarantee that it produces valid code, but we would prefer that it does. I do not think we yet have a clear argument that it's worth accepting costs elsewhere in order to solely improve `-ast-print`, but fortunately most improvements to `-ast-print` also improve our diagnostic quality, the usability of our AST in tooling scenarios, or some other property that we do care about, so this issue seldom arises. (For example, in the context of https://reviews.llvm.org/D45463, information on whether a declaration owns a `TagDecl` is directly useful to tooling (for instance, refactoring tools care how a multi-declarator declaration was written so that they can properly rewrite it), as well as being useful for `-ast-print` fidelity. So it would make sense to consider how to better represent this information in the AST so that all the interested users have access to it.)

Thanks.  That helps.

> However... Clang is an open-source meritocracy. As such, the goals of the Clang project are a synthesized amalgam of the goals of the Clang contributors. If you want to take ownership of the AST printer and, for instance, make it always generate valid code, we can discuss the technical merits of that (use cases, benefit to you and to other users, maintenance costs, etc) even if the original purpose of the flag did not extend that far.

For the moment, I'll just fix issues as I encounter them.  If I decide -ast-print fidelity is vital to my work in the long term, or if my fixes introduce problematic costs so that our goals for -ast-print then conflict, then we can have that larger discussion.

I'm not sure I'm experienced enough to take ownership of -ast-print just yet.  However, if there are as many fidelity issues as you've suggested, I'll probably get there eventually.


https://reviews.llvm.org/D45093