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

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

In https://reviews.llvm.org/D45463#1087174, @rsmith wrote:

> > TagSpecifierAs expands sizeof(PrintingPolicy) from 8 to 16 bytes. My concern is the comments on PrintingPolicy about how PrintingPolicy is intended to be small. My guess it that 16 bytes is still small enough, but perhaps Richard Smith, who wrote that comment, knows better.
>
> This seems fine. See r270009 for some background for that comment -- we used to store a copy of the `LangOptions` in the `PrintingPolicy` (among other things), and copying these objects (including the potentially-large vectors within the `LangOptions`) was a double-digit percentage of the compile time of some compilations. That's a very different ball park from a change from 8 bytes to 16 bytes.


Makes sense.  Thanks.



================
Comment at: include/clang-c/Index.h:4116
   CXPrintingPolicy_SuppressTagKeyword,
-  CXPrintingPolicy_IncludeTagDefinition,
   CXPrintingPolicy_SuppressScope,
----------------
rsmith wrote:
> This is not OK: this would be an ABI break for the stable libclang ABI.
Understood.  We could keep IncludeTagDefinition for that purpose and just replace its current internal use with TagSpecifierAs.

Out of curiosity, do we know how others are using IncludeTagDefinition?


================
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:
> 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.


https://reviews.llvm.org/D45463