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

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

jdenny created this revision.
jdenny added reviewers: rsmith, nik, jbcoe, aaron.ballman, hfinkel.

For example, given:

  void fn() {
    struct T *p0;
    struct T { int i; } *p1;
  }

-ast-print produced:

  void fn() {
    struct T { int i; } *p0;
    struct T { int i; } *p1;
  }

Compiling that fails with a redefinition error.

Details:

For a tag specifier (that is, struct/union/class/enum used as a type
specifier in a declaration) that was also a tag declaration (that is, 
first occurrence of the tag) or tag redeclaration (that is, later
occurrence that specifies attributes or a member list), clang printed
the tag specifier as either (1) the full tag definition if one 
existed, or (2) the first tag declaration otherwise.  As in the above
example, redefinition errors were sometimes introduced.  Even when
that was impossible because no member list was ever specified,
attributes were sometimes lost, thus changing semantics and 
diagnostics.

This patch fixes a major culprit for these problems.  It does so by
replacing PrintingPolicy's IncludeTagDefinition, which triggered
printing of the member list, attributes, etc. for a tag specifier by
using a tag (re)declaration selected as described above.  The 
replacement is TagSpecifierAs, which triggers the same thing except it
specifies the tag (re)declaration actually recorded by the parser for 
that tag specifier.

I have two concerns about this patch:

1. 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.

2. This patch drops IncludeTagDefinition from CXPrintingPolicyProperty and does not attempt to replace it because TagSpecifierAs doesn't seem to fit that interface.  This bit of functionality seems internal to the printing design, but I don't have any experience with CXPrintingPolicyProperty, so I might be missing an important use case.  We could actually keep IncludeTagDefinition alongside TagSpecifierAs, but my naive guess is that IncludeTagDefinition is not reliable enough to be worthwhile.  Perhaps Nikolai Kosjar or Jonathan Coe, who wrote/reviewed CXPrintingPolicyProperty can comment.


https://reviews.llvm.org/D45463

Files:
  include/clang-c/Index.h
  include/clang/AST/PrettyPrinter.h
  lib/AST/DeclPrinter.cpp
  lib/AST/TypePrinter.cpp
  test/Misc/ast-print-enum-decl.c
  test/Misc/ast-print-record-decl.c
  tools/c-index-test/c-index-test.c
  tools/libclang/CIndex.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D45463.141744.patch
Type: text/x-patch
Size: 18759 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180409/3630e33c/attachment-0001.bin>