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