jdenny 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;
----------------
rsmith wrote:
> rsmith wrote:
> > 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.
> > 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.
>
> Ah, no, the problem is a little deeper than that; we don't even create
> distinct type sugar for the new-decl vs existing-decl cases. Maybe that'd be
> somewhere to start -- perhaps the `ElaboratedType` node could track that for
> us.
> 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.
I've heard that, but it seemed to me that what's good for the latter is likely
good for the former.
Still, I don't want to waste time on patches that won't be accepted because
they're harder to justify purely for debugging purposes. Is there a better
place to explore pretty printing valid source code from a clang AST?
> 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.
I haven't considered cases where diagnostics are broken by this issue. For
diagnostics, is it important to print the exact set of attributes on a
particular declaration rather than to print, say, the entire list of attributes
from all declarations of the tag? The latter appears to be easier to implement.
> Ah, no, the problem is a little deeper than that; we don't even create
> distinct type sugar for the new-decl vs existing-decl cases. Maybe that'd be
> somewhere to start -- perhaps the ElaboratedType node could track that for us.
My approach was to constrain my changes to the -ast-print implementation as
much as possible without changing the AST in order to avoid breaking something
else. I found that I could fix a big problem (printing the right tag decl at
each specifier) just using the AST as it's already constructed. DeclGroup
gives us the info we need. That info just isn't in the most convenient place,
as I think you're pointing out. (Also keep in mind that there's a child patch
that fixes decl group discovery in some contexts not exercised by this patch.)
If you feel that ElaboratedType is a better way to go, I'll look into it and
get back to you later.
https://reviews.llvm.org/D45463
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
