[PATCH] D45463: [AST] Print correct tag decl for tag specifier

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: lib/AST/DeclPrinter.cpp:180-181
     if (isFirst) {
-      if(TD)
-        SubPolicy.IncludeTagDefinition = true;
+      if (TD)
+        SubPolicy.TagSpecifierAs = TD;
       SubPolicy.SuppressSpecifiers = false;
----------------
aaron.ballman wrote:
> jdenny wrote:
> > rsmith wrote:
> > > Instead of the changes in this patch, can you address your use case by just changing the `if` here to `if (TD && TD->isThisDeclarationADefinition())`?
> > It sounds like that would fix the case of duplicated definitions.
> > 
> > It doesn't address the case of dropped or relocated attributes.  For example, -ast-print drops the following attribute and thus the deprecation warning:
> > 
> > ```
> > void fn() {
> >   struct T *p0;
> >   struct __attribute__((deprecated)) T *p1;
> > }
> > ```
> > 
> > I don't know how clang *should* accept and interpret attributes in such cases.  However, it seems wrong for -ast-print to produce a program that clang interprets differently than the original program.
> > I don't know how clang *should* accept and interpret attributes in such cases. However, it seems wrong for -ast-print to produce a program that clang interprets differently than the original program.
> 
> This has been a *very* long-standing issue with -ast-print that I'd love to see fixed. The AST printer does not print attributes in the correct locations except by accident, and it oftentimes prints with the wrong syntax, or is missing from places where it should be printed.
> 
> Long story short, I agree that this behavior is wrong and needs to be fixed, but it's also going to take a bit of work to get it right.
> it seems wrong for -ast-print to produce a program that clang interprets differently than the original program.

Historically we've viewed `-ast-print` as a debugging tool, not as a way of producing valid source code. If you want to change that, there will be a loooong list of issues to fix. However, one of the important and intended uses of the `-ast-print` mechanism is producing pretty-printed type names and the like for diagnostics, and in that context, it *is* important to print attributes properly.

> It doesn't address the case of dropped or relocated attributes.

Hmm. A big part of the problem here, in my view, is that when we come to print a `TagType`, we call `TagType::getDecl`, which returns our "favourite" declaration of that entity, not necessarily the one that was named (or even created) for that instance of the type. We could easily fix that (by adding an accessor to reach `TagType::decl` rather than calling `TagType::getDecl()`), and perhaps *that* plus the current tracking of whether we're printing the tag declaration that is owned by the current `DeclGroup` would be enough to get correct displays here.

Another part of the problem is that `TagType` does not track whether it's referring to its "own" `TagDecl` (ie, one created when parsing that type) or some pre-existing one, and that affects whether and how attributes should be printed. That's why we need the `IncludeTagDefinition` flag in the first place; it's our attempt to recreate knowledge about whether the `TagType` owns its `TagDecl`. We could consider instead tracking a flag on `TagType` sugar nodes to model that, which would let us get rid of that flag.


https://reviews.llvm.org/D45463