rsmith added a comment.

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.

(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'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.)

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.


https://reviews.llvm.org/D45093



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to