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