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