[PATCH] D141714: Fix ast print of variables with attributes

[Giuliano Belinassi via Phabricator via cfe-commits]

giulianobelinassi added inline comments.



Comment at: clang/lib/AST/DeclPrinter.cpp:269-276
+static bool mustPrintOnLeftSide(attr::Kind kind) {
+  switch (kind) {
+#include "clang/Basic/AttrLeftSideMustPrintList.inc"
+return true;
+  default:
+return false;
+  }

aaron.ballman wrote:
> These changes have caused a new warning to show up in MSVC builds: `switch 
> statement contains 'default' but no 'case' labels`
> 
> There's a few possible ways to resolve this: tablegen the entire switch 
> statement (so you can elide the switch if there are no case statements and 
> just make it `return false;` in that case), or remove the switch and add it 
> back once the .inc file is no longer empty (this is a bit fragile for my 
> tastes), or find an attribute that needs to be marked as must print on the 
> left so the .inc file is no longer empty.
> 
> Can you look into solving this? I've not seen a bot go red from the change 
> yet, but we do have warnings-as-error builds with MSVC (at least in 
> downstreams) so the extra warning here is a problem.
> tablegen the entire switch statement (so you can elide the switch if there 
> are no case statements and just make it return false; in that case)

I will do this. This would IMHO give more control to the tool generating the 
table, which I think should have been the original approach. In any case, I 
think I will be able to come up with a patch in the next hours.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141714: Fix ast print of variables with attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.






Comment at: clang/lib/AST/DeclPrinter.cpp:269-276
+static bool mustPrintOnLeftSide(attr::Kind kind) {
+  switch (kind) {
+#include "clang/Basic/AttrLeftSideMustPrintList.inc"
+return true;
+  default:
+return false;
+  }

These changes have caused a new warning to show up in MSVC builds: `switch 
statement contains 'default' but no 'case' labels`

There's a few possible ways to resolve this: tablegen the entire switch 
statement (so you can elide the switch if there are no case statements and just 
make it `return false;` in that case), or remove the switch and add it back 
once the .inc file is no longer empty (this is a bit fragile for my tastes), or 
find an attribute that needs to be marked as must print on the left so the .inc 
file is no longer empty.

Can you look into solving this? I've not seen a bot go red from the change yet, 
but we do have warnings-as-error builds with MSVC (at least in downstreams) so 
the extra warning here is a problem.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141714: Fix ast print of variables with attributes

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/AST/DeclPrinter.cpp:312
+  // printing on the left side for readbility.
+else if (const VarDecl *VD = dyn_cast(D);
+   VD && VD->getInit() &&

giulianobelinassi wrote:
> erichkeane wrote:
> > While compiling this, I discovered that the lack of curley braces on the 
> > `else if (canPrintOnLeftSide(A))` means that this `else-if` is actually an 
> > else to THAT instead, despite its indention (which implies it should be 
> > associated with the `if (const FunctionDecl...`).
> > 
> > Which is it?  I presume the indenting is what you mean, but it hasn't been 
> > tested in a way that matters.  Can you add a test that validates the 
> > var-decl printing on the LHS ONLY happens when it 'can' print on the LHS?
> Sorry for the warning there. I will post a fixed version of this as soon as 
> my build with `-Werror=all` completes.
> 
> Just to be clear, the intended code with braces is:
> ```
>   AttrPrintLoc AttrLoc = AttrPrintLoc::Right;
>   if (mustPrintOnLeftSide(A))
> // If we must always print on left side (e.g. declspec), then mark as
> // so.
> AttrLoc = AttrPrintLoc::Left;
>   else if (canPrintOnLeftSide(A)) {
> // For functions with body defined we print the attributes on the left
> // side so that GCC accept our dumps as well.
> if (const FunctionDecl *FD = dyn_cast(D);
> FD && FD->isThisDeclarationADefinition())
>   // In case Decl is a function with a body, then attrs should be 
> print
>   // on the left side.
>   AttrLoc = AttrPrintLoc::Left;
> 
>   // In case it is a variable declaration with a ctor, then allow
>   // printing on the left side for readbility.
> else if (const VarDecl *VD = dyn_cast(D);
>VD && VD->getInit() &&
>VD->getInitStyle() == VarDecl::CallInit)
>   AttrLoc = AttrPrintLoc::Left;
>   }
>   // Only print the side matches the user requested.
>   if ((Loc & AttrLoc) != AttrPrintLoc::None)
> A->printPretty(Out, Policy);
> ```
> 
> As you can see, both `if (const FunctionDecl *FD` and `else if (const VarDecl 
> *VD` are inside the  `if (canPrintOnLeftSide())`, and the `AttrLoc` variable 
> is only set to `AttrPrintLoc::Left` when this function returns `true`. Hence 
> unless there is a bug in `canPrintOnLeftSide` or `mustPrintOnLeftSide` (which 
> I don't see any), I can't see a case where PrintOnLeft is false and it prints 
> on the left side.
> 
> Now, I could be wrong on this, but IIRC the brackets on this is optional once 
> the `else if` would match the preceding else-less if, which in this case 
> would be `if (const FunctionDecl *FD = dyn_cast(D);`, which is 
> what the indentation meant.
yep, you're right, it WAS already behaving how it was supposed to.  Committed.  
Please keep an eye out for failed buildbots/etc.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141714: Fix ast print of variables with attributes

[Erich Keane via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG46f3ade5083b: Fix ast print of variables with attributes 
(authored by giulianobelinassi, committed by erichkeane).

Changed prior to commit:
  https://reviews.llvm.org/D141714?vs=556197=556201#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/CMakeLists.txt
  clang/lib/AST/DeclPrinter.cpp
  clang/test/AST/ast-print-attr-knr.c
  clang/test/AST/ast-print-attr.c
  clang/test/AST/ast-print-method-decl.cpp
  clang/test/AST/ast-print-pragmas.cpp
  clang/test/Analysis/blocks.mm
  clang/test/OpenMP/assumes_codegen.cpp
  clang/test/OpenMP/assumes_print.cpp
  clang/test/OpenMP/assumes_template_print.cpp
  clang/test/OpenMP/declare_simd_ast_print.cpp
  clang/test/Sema/attr-print.c
  clang/test/SemaCXX/attr-print.cpp
  clang/test/SemaCXX/cxx11-attr-print.cpp
  clang/utils/TableGen/ClangAttrEmitter.cpp
  clang/utils/TableGen/TableGen.cpp
  clang/utils/TableGen/TableGenBackends.h

Index: clang/utils/TableGen/TableGenBackends.h
===
--- clang/utils/TableGen/TableGenBackends.h
+++ clang/utils/TableGen/TableGenBackends.h
@@ -39,6 +39,8 @@
 void EmitClangAttrClass(llvm::RecordKeeper , llvm::raw_ostream );
 void EmitClangAttrImpl(llvm::RecordKeeper , llvm::raw_ostream );
 void EmitClangAttrList(llvm::RecordKeeper , llvm::raw_ostream );
+void EmitClangAttrPrintList(const std::string ,
+llvm::RecordKeeper , llvm::raw_ostream );
 void EmitClangAttrSubjectMatchRuleList(llvm::RecordKeeper ,
llvm::raw_ostream );
 void EmitClangAttrPCHRead(llvm::RecordKeeper , llvm::raw_ostream );
Index: clang/utils/TableGen/TableGen.cpp
===
--- clang/utils/TableGen/TableGen.cpp
+++ clang/utils/TableGen/TableGen.cpp
@@ -31,6 +31,8 @@
   GenClangAttrSubjectMatchRulesParserStringSwitches,
   GenClangAttrImpl,
   GenClangAttrList,
+  GenClangAttrCanPrintLeftList,
+  GenClangAttrMustPrintLeftList,
   GenClangAttrDocTable,
   GenClangAttrSubjectMatchRuleList,
   GenClangAttrPCHRead,
@@ -127,6 +129,14 @@
"Generate clang attribute implementations"),
 clEnumValN(GenClangAttrList, "gen-clang-attr-list",
"Generate a clang attribute list"),
+clEnumValN(GenClangAttrCanPrintLeftList,
+   "gen-clang-attr-can-print-left-list",
+   "Generate list of attributes that can be printed on left "
+   "side of a decl"),
+clEnumValN(GenClangAttrMustPrintLeftList,
+   "gen-clang-attr-must-print-left-list",
+   "Generate list of attributes that must be printed on left "
+   "side of a decl"),
 clEnumValN(GenClangAttrDocTable, "gen-clang-attr-doc-table",
"Generate a table of attribute documentation"),
 clEnumValN(GenClangAttrSubjectMatchRuleList,
@@ -315,6 +325,12 @@
   case GenClangAttrList:
 EmitClangAttrList(Records, OS);
 break;
+  case GenClangAttrCanPrintLeftList:
+EmitClangAttrPrintList("CanPrintOnLeft", Records, OS);
+break;
+  case GenClangAttrMustPrintLeftList:
+EmitClangAttrPrintList("PrintOnLeft", Records, OS);
+break;
   case GenClangAttrDocTable:
 EmitClangAttrDocTable(Records, OS);
 break;
Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -3207,6 +3207,27 @@
   OS << "#undef PRAGMA_SPELLING_ATTR\n";
 }
 
+// Emits the enumeration list for attributes.
+void EmitClangAttrPrintList(const std::string , RecordKeeper ,
+raw_ostream ) {
+  emitSourceFileHeader(
+  "List of attributes that can be print on the left side of a decl", OS);
+
+  AttrClassHierarchy Hierarchy(Records);
+
+  std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
+  std::vector PragmaAttrs;
+  for (auto *Attr : Attrs) {
+if (!Attr->getValueAsBit("ASTNode"))
+  continue;
+
+if (!Attr->getValueAsBit(FieldName))
+  continue;
+
+OS << "case attr::" << Attr->getName() << ":\n";
+  }
+}
+
 // Emits the enumeration list for attributes.
 void EmitClangAttrSubjectMatchRuleList(RecordKeeper , raw_ostream ) {
   emitSourceFileHeader(
Index: clang/test/SemaCXX/cxx11-attr-print.cpp
===
--- clang/test/SemaCXX/cxx11-attr-print.cpp
+++ clang/test/SemaCXX/cxx11-attr-print.cpp
@@ -3,8 +3,7 @@
 // CHECK: int x __attribute__((aligned(4)));
 int x __attribute__((aligned(4)));
 
-// FIXME: Print this at a valid 

[PATCH] D141714: Fix ast print of variables with attributes

[Giuliano Belinassi via Phabricator via cfe-commits]

giulianobelinassi updated this revision to Diff 556197.
giulianobelinassi added a comment.

Fix `dangling-else` compilation warning.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/CMakeLists.txt
  clang/lib/AST/DeclPrinter.cpp
  clang/test/AST/ast-print-attr-knr.c
  clang/test/AST/ast-print-attr.c
  clang/test/AST/ast-print-method-decl.cpp
  clang/test/AST/ast-print-pragmas.cpp
  clang/test/Analysis/blocks.mm
  clang/test/OpenMP/assumes_codegen.cpp
  clang/test/OpenMP/assumes_print.cpp
  clang/test/OpenMP/assumes_template_print.cpp
  clang/test/OpenMP/declare_simd_ast_print.cpp
  clang/test/Sema/attr-print.c
  clang/test/SemaCXX/attr-print.cpp
  clang/test/SemaCXX/cxx11-attr-print.cpp
  clang/utils/TableGen/ClangAttrEmitter.cpp
  clang/utils/TableGen/TableGen.cpp
  clang/utils/TableGen/TableGenBackends.h

Index: clang/utils/TableGen/TableGenBackends.h
===
--- clang/utils/TableGen/TableGenBackends.h
+++ clang/utils/TableGen/TableGenBackends.h
@@ -39,6 +39,8 @@
 void EmitClangAttrClass(llvm::RecordKeeper , llvm::raw_ostream );
 void EmitClangAttrImpl(llvm::RecordKeeper , llvm::raw_ostream );
 void EmitClangAttrList(llvm::RecordKeeper , llvm::raw_ostream );
+void EmitClangAttrPrintList(const std::string ,
+llvm::RecordKeeper , llvm::raw_ostream );
 void EmitClangAttrSubjectMatchRuleList(llvm::RecordKeeper ,
llvm::raw_ostream );
 void EmitClangAttrPCHRead(llvm::RecordKeeper , llvm::raw_ostream );
Index: clang/utils/TableGen/TableGen.cpp
===
--- clang/utils/TableGen/TableGen.cpp
+++ clang/utils/TableGen/TableGen.cpp
@@ -31,6 +31,8 @@
   GenClangAttrSubjectMatchRulesParserStringSwitches,
   GenClangAttrImpl,
   GenClangAttrList,
+  GenClangAttrCanPrintLeftList,
+  GenClangAttrMustPrintLeftList,
   GenClangAttrDocTable,
   GenClangAttrSubjectMatchRuleList,
   GenClangAttrPCHRead,
@@ -127,6 +129,14 @@
"Generate clang attribute implementations"),
 clEnumValN(GenClangAttrList, "gen-clang-attr-list",
"Generate a clang attribute list"),
+clEnumValN(GenClangAttrCanPrintLeftList,
+   "gen-clang-attr-can-print-left-list",
+   "Generate list of attributes that can be printed on left "
+   "side of a decl"),
+clEnumValN(GenClangAttrMustPrintLeftList,
+   "gen-clang-attr-must-print-left-list",
+   "Generate list of attributes that must be printed on left "
+   "side of a decl"),
 clEnumValN(GenClangAttrDocTable, "gen-clang-attr-doc-table",
"Generate a table of attribute documentation"),
 clEnumValN(GenClangAttrSubjectMatchRuleList,
@@ -315,6 +325,12 @@
   case GenClangAttrList:
 EmitClangAttrList(Records, OS);
 break;
+  case GenClangAttrCanPrintLeftList:
+EmitClangAttrPrintList("CanPrintOnLeft", Records, OS);
+break;
+  case GenClangAttrMustPrintLeftList:
+EmitClangAttrPrintList("PrintOnLeft", Records, OS);
+break;
   case GenClangAttrDocTable:
 EmitClangAttrDocTable(Records, OS);
 break;
Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -3207,6 +3207,27 @@
   OS << "#undef PRAGMA_SPELLING_ATTR\n";
 }
 
+// Emits the enumeration list for attributes.
+void EmitClangAttrPrintList(const std::string , RecordKeeper ,
+raw_ostream ) {
+  emitSourceFileHeader(
+  "List of attributes that can be print on the left side of a decl", OS);
+
+  AttrClassHierarchy Hierarchy(Records);
+
+  std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
+  std::vector PragmaAttrs;
+  for (auto *Attr : Attrs) {
+if (!Attr->getValueAsBit("ASTNode"))
+  continue;
+
+if (!Attr->getValueAsBit(FieldName))
+  continue;
+
+OS << "case attr::" << Attr->getName() << ":\n";
+  }
+}
+
 // Emits the enumeration list for attributes.
 void EmitClangAttrSubjectMatchRuleList(RecordKeeper , raw_ostream ) {
   emitSourceFileHeader(
Index: clang/test/SemaCXX/cxx11-attr-print.cpp
===
--- clang/test/SemaCXX/cxx11-attr-print.cpp
+++ clang/test/SemaCXX/cxx11-attr-print.cpp
@@ -3,8 +3,7 @@
 // CHECK: int x __attribute__((aligned(4)));
 int x __attribute__((aligned(4)));
 
-// FIXME: Print this at a valid location for a __declspec attr.
-// CHECK: int y __declspec(align(4));
+// CHECK: __declspec(align(4)) int y;
 __declspec(align(4)) int y;
 
 // CHECK: int z {{\[}}[gnu::aligned(4)]];
@@ -66,7 +65,7 @@
 // CHECK: 

[PATCH] D141714: Fix ast print of variables with attributes

[Giuliano Belinassi via Phabricator via cfe-commits]

giulianobelinassi added inline comments.



Comment at: clang/lib/AST/DeclPrinter.cpp:312
+  // printing on the left side for readbility.
+else if (const VarDecl *VD = dyn_cast(D);
+   VD && VD->getInit() &&

erichkeane wrote:
> While compiling this, I discovered that the lack of curley braces on the 
> `else if (canPrintOnLeftSide(A))` means that this `else-if` is actually an 
> else to THAT instead, despite its indention (which implies it should be 
> associated with the `if (const FunctionDecl...`).
> 
> Which is it?  I presume the indenting is what you mean, but it hasn't been 
> tested in a way that matters.  Can you add a test that validates the var-decl 
> printing on the LHS ONLY happens when it 'can' print on the LHS?
Sorry for the warning there. I will post a fixed version of this as soon as my 
build with `-Werror=all` completes.

Just to be clear, the intended code with braces is:
```
  AttrPrintLoc AttrLoc = AttrPrintLoc::Right;
  if (mustPrintOnLeftSide(A))
// If we must always print on left side (e.g. declspec), then mark as
// so.
AttrLoc = AttrPrintLoc::Left;
  else if (canPrintOnLeftSide(A)) {
// For functions with body defined we print the attributes on the left
// side so that GCC accept our dumps as well.
if (const FunctionDecl *FD = dyn_cast(D);
FD && FD->isThisDeclarationADefinition())
  // In case Decl is a function with a body, then attrs should be print
  // on the left side.
  AttrLoc = AttrPrintLoc::Left;

  // In case it is a variable declaration with a ctor, then allow
  // printing on the left side for readbility.
else if (const VarDecl *VD = dyn_cast(D);
   VD && VD->getInit() &&
   VD->getInitStyle() == VarDecl::CallInit)
  AttrLoc = AttrPrintLoc::Left;
  }
  // Only print the side matches the user requested.
  if ((Loc & AttrLoc) != AttrPrintLoc::None)
A->printPretty(Out, Policy);
```

As you can see, both `if (const FunctionDecl *FD` and `else if (const VarDecl 
*VD` are inside the  `if (canPrintOnLeftSide())`, and the `AttrLoc` variable is 
only set to `AttrPrintLoc::Left` when this function returns `true`. Hence 
unless there is a bug in `canPrintOnLeftSide` or `mustPrintOnLeftSide` (which I 
don't see any), I can't see a case where PrintOnLeft is false and it prints on 
the left side.

Now, I could be wrong on this, but IIRC the brackets on this is optional once 
the `else if` would match the preceding else-less if, which in this case would 
be `if (const FunctionDecl *FD = dyn_cast(D);`, which is what the 
indentation meant.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141714: Fix ast print of variables with attributes

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/AST/DeclPrinter.cpp:312
+  // printing on the left side for readbility.
+else if (const VarDecl *VD = dyn_cast(D);
+   VD && VD->getInit() &&

While compiling this, I discovered that the lack of curley braces on the `else 
if (canPrintOnLeftSide(A))` means that this `else-if` is actually an else to 
THAT instead, despite its indention (which implies it should be associated with 
the `if (const FunctionDecl...`).

Which is it?  I presume the indenting is what you mean, but it hasn't been 
tested in a way that matters.  Can you add a test that validates the var-decl 
printing on the LHS ONLY happens when it 'can' print on the LHS?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141714: Fix ast print of variables with attributes

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D141714#4640825 , 
@giulianobelinassi wrote:

> In D141714#4638225 , @erichkeane 
> wrote:
>
>> Looks fine to me, please don't commit for a day or two to give 
>> @aaron.ballman a chance to make a final comment.
>
> I am sorry, but how am I supposed to commit those changes to main if I do not 
> have write permission?

I didn't realize you didn't have commit permissions!  If you send me (either 
here, or privately) your name and email in the form of: "Full Name 
"  I can do it for you


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141714: Fix ast print of variables with attributes

[Giuliano Belinassi via Phabricator via cfe-commits]

giulianobelinassi added a comment.

In D141714#4638225 , @erichkeane 
wrote:

> Looks fine to me, please don't commit for a day or two to give @aaron.ballman 
> a chance to make a final comment.

I am sorry, but how am I supposed to commit those changes to main if I do not 
have write permission?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141714: Fix ast print of variables with attributes

[Erich Keane via Phabricator via cfe-commits]

erichkeane accepted this revision.
erichkeane added a comment.
This revision is now accepted and ready to land.

Looks fine to me, please don't commit for a day or two to give @aaron.ballman a 
chance to make a final comment.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141714: Fix ast print of variables with attributes

[Giuliano Belinassi via Phabricator via cfe-commits]

giulianobelinassi updated this revision to Diff 555642.
giulianobelinassi added a comment.

Apply @erichkeane suggestions


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/CMakeLists.txt
  clang/lib/AST/DeclPrinter.cpp
  clang/test/AST/ast-print-attr-knr.c
  clang/test/AST/ast-print-attr.c
  clang/test/AST/ast-print-method-decl.cpp
  clang/test/AST/ast-print-pragmas.cpp
  clang/test/Analysis/blocks.mm
  clang/test/OpenMP/assumes_codegen.cpp
  clang/test/OpenMP/assumes_print.cpp
  clang/test/OpenMP/assumes_template_print.cpp
  clang/test/OpenMP/declare_simd_ast_print.cpp
  clang/test/Sema/attr-print.c
  clang/test/SemaCXX/attr-print.cpp
  clang/test/SemaCXX/cxx11-attr-print.cpp
  clang/utils/TableGen/ClangAttrEmitter.cpp
  clang/utils/TableGen/TableGen.cpp
  clang/utils/TableGen/TableGenBackends.h

Index: clang/utils/TableGen/TableGenBackends.h
===
--- clang/utils/TableGen/TableGenBackends.h
+++ clang/utils/TableGen/TableGenBackends.h
@@ -39,6 +39,8 @@
 void EmitClangAttrClass(llvm::RecordKeeper , llvm::raw_ostream );
 void EmitClangAttrImpl(llvm::RecordKeeper , llvm::raw_ostream );
 void EmitClangAttrList(llvm::RecordKeeper , llvm::raw_ostream );
+void EmitClangAttrPrintList(const std::string ,
+llvm::RecordKeeper , llvm::raw_ostream );
 void EmitClangAttrSubjectMatchRuleList(llvm::RecordKeeper ,
llvm::raw_ostream );
 void EmitClangAttrPCHRead(llvm::RecordKeeper , llvm::raw_ostream );
Index: clang/utils/TableGen/TableGen.cpp
===
--- clang/utils/TableGen/TableGen.cpp
+++ clang/utils/TableGen/TableGen.cpp
@@ -31,6 +31,8 @@
   GenClangAttrSubjectMatchRulesParserStringSwitches,
   GenClangAttrImpl,
   GenClangAttrList,
+  GenClangAttrCanPrintLeftList,
+  GenClangAttrMustPrintLeftList,
   GenClangAttrDocTable,
   GenClangAttrSubjectMatchRuleList,
   GenClangAttrPCHRead,
@@ -127,6 +129,14 @@
"Generate clang attribute implementations"),
 clEnumValN(GenClangAttrList, "gen-clang-attr-list",
"Generate a clang attribute list"),
+clEnumValN(GenClangAttrCanPrintLeftList,
+   "gen-clang-attr-can-print-left-list",
+   "Generate list of attributes that can be printed on left "
+   "side of a decl"),
+clEnumValN(GenClangAttrMustPrintLeftList,
+   "gen-clang-attr-must-print-left-list",
+   "Generate list of attributes that must be printed on left "
+   "side of a decl"),
 clEnumValN(GenClangAttrDocTable, "gen-clang-attr-doc-table",
"Generate a table of attribute documentation"),
 clEnumValN(GenClangAttrSubjectMatchRuleList,
@@ -315,6 +325,12 @@
   case GenClangAttrList:
 EmitClangAttrList(Records, OS);
 break;
+  case GenClangAttrCanPrintLeftList:
+EmitClangAttrPrintList("CanPrintOnLeft", Records, OS);
+break;
+  case GenClangAttrMustPrintLeftList:
+EmitClangAttrPrintList("PrintOnLeft", Records, OS);
+break;
   case GenClangAttrDocTable:
 EmitClangAttrDocTable(Records, OS);
 break;
Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -3207,6 +3207,27 @@
   OS << "#undef PRAGMA_SPELLING_ATTR\n";
 }
 
+// Emits the enumeration list for attributes.
+void EmitClangAttrPrintList(const std::string , RecordKeeper ,
+raw_ostream ) {
+  emitSourceFileHeader(
+  "List of attributes that can be print on the left side of a decl", OS);
+
+  AttrClassHierarchy Hierarchy(Records);
+
+  std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
+  std::vector PragmaAttrs;
+  for (auto *Attr : Attrs) {
+if (!Attr->getValueAsBit("ASTNode"))
+  continue;
+
+if (!Attr->getValueAsBit(FieldName))
+  continue;
+
+OS << "case attr::" << Attr->getName() << ":\n";
+  }
+}
+
 // Emits the enumeration list for attributes.
 void EmitClangAttrSubjectMatchRuleList(RecordKeeper , raw_ostream ) {
   emitSourceFileHeader(
Index: clang/test/SemaCXX/cxx11-attr-print.cpp
===
--- clang/test/SemaCXX/cxx11-attr-print.cpp
+++ clang/test/SemaCXX/cxx11-attr-print.cpp
@@ -3,8 +3,7 @@
 // CHECK: int x __attribute__((aligned(4)));
 int x __attribute__((aligned(4)));
 
-// FIXME: Print this at a valid location for a __declspec attr.
-// CHECK: int y __declspec(align(4));
+// CHECK: __declspec(align(4)) int y;
 __declspec(align(4)) int y;
 
 // CHECK: int z {{\[}}[gnu::aligned(4)]];
@@ -66,7 +65,7 @@
 // CHECK: int m 

[PATCH] D141714: Fix ast print of variables with attributes

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

I think we can do better naming the tablegen'ed parts, else a bunch of smaller 
changes.  Approach seems good enough to me, though Aaron should scroll 
through/make a determination after you've fixed my concerns.




Comment at: clang/include/clang/Basic/Attr.td:565
 class Attr {
+  // On a declaration, can this attribute be print on the left side?
+  bit CanPrintOnLeftSide = 1;





Comment at: clang/include/clang/Basic/Attr.td:566
+  // On a declaration, can this attribute be print on the left side?
+  bit CanPrintOnLeftSide = 1;
+  // On a declaration, must this attribute be print on the left side?





Comment at: clang/include/clang/Basic/Attr.td:567
+  bit CanPrintOnLeftSide = 1;
+  // On a declaration, must this attribute be print on the left side?
+  bit MustPrintOnLeftSide = 0;





Comment at: clang/include/clang/Basic/Attr.td:568
+  // On a declaration, must this attribute be print on the left side?
+  bit MustPrintOnLeftSide = 0;
   // The various ways in which an attribute can be spelled in source





Comment at: clang/lib/AST/DeclPrinter.cpp:263
+static bool canPrintOnLeftSide(const Attr *A) {
+  if (A->isStandardAttributeSyntax()) {
+return false;

Don't include curley brackets here.



Comment at: clang/lib/AST/DeclPrinter.cpp:280
+static bool mustPrintOnLeftSide(const Attr *A) {
+  if (A->isDeclspecAttribute()) {
+return true;

Same here, don't use curleys on single-liners.



Comment at: clang/lib/AST/DeclPrinter.cpp:298
+
+  AttrPrintLoc attrloc = AttrPrintLoc::Right;
+  if (mustPrintOnLeftSide(A)) {





Comment at: clang/lib/AST/DeclPrinter.cpp:306
+// side so that GCC accept our dumps as well.
+if (const FunctionDecl *FD = dyn_cast(D);
+FD && FD->isThisDeclarationADefinition()) {





Comment at: clang/lib/AST/DeclPrinter.cpp:314
+  // printing on the left side for readbility.
+} else if (const VarDecl *VD = dyn_cast(D);
+   VD && VD->getInit() &&





Comment at: clang/lib/AST/DeclPrinter.cpp:1003
 
-  printDeclType(T, (isa(D) && Policy.CleanUglifiedParameters &&
-D->getIdentifier())
-   ? D->getIdentifier()->deuglifiedName()
-   : D->getName());
+  std::string Name;
+

Instead of making this a std::string, make it a StringRef and just assign it on 
line 1005, it looks like both sides of that return a StringRef (deuglifiedName 
and getName), and your += is unnecessary (since there is nothing in Name here 
anyway).



Comment at: clang/utils/TableGen/TableGen.cpp:282
"Generate riscv_vector_builtin_sema.inc for clang"),
-clEnumValN(GenRISCVSiFiveVectorBuiltins, 
"gen-riscv-sifive-vector-builtins",
+clEnumValN(GenRISCVSiFiveVectorBuiltins,
+   "gen-riscv-sifive-vector-builtins",

Unrelated changes, please remove.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141714: Fix ast print of variables with attributes

[Giuliano Belinassi via Phabricator via cfe-commits]

giulianobelinassi added a comment.

Ping.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141714: Fix ast print of variables with attributes

[Timo Stripf via Phabricator via cfe-commits]

strimo378 added a comment.

@aaron.ballman I had today a teams meeting with @giulianobelinassi and we 
discussed both patches as well as that we want to work together in improving 
AST print. Both patches are fine for us since they improve the attribute 
printing. The features of the respective other patch can be easily integrated 
with minimal changes in a subsequent smaller patch. We further think that there 
are a lot more cases of attributes that we need to consider that we do not know 
atm.

I think we should go one with patch that has the higher probability to get 
landed soon. Since this branch had a lot more reviews, I suggest we go further 
with this patch. I further suggest to keep it simple and remove the tblgen part 
and add it in an subsequent patch once we know all cases and requirements. What 
do you think?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141714: Fix ast print of variables with attributes

[Timo Stripf via Phabricator via cfe-commits]

strimo378 added a comment.

@giulianobelinassi I provided a similar patch as you addressed here, see 
https://reviews.llvm.org/D157394 . It looks like we have the same requirement 
that we both need compilable ast-print code. Are you interested in a teams 
meeting to discuss the topic and align our effort? If yes please write me an 
mail to timo.str...@emmtrix.com .


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141714: Fix ast print of variables with attributes

[Giuliano Belinassi via Phabricator via cfe-commits]

giulianobelinassi added a comment.

Hello,

It took me a while, but here it is the newer version of the patch with the 
tablegen stuff. Please reach to me if something needs to be changed in this 
regard.

This also improves the readability of declarations of variables that have a 
parenthesis constructor (like the __block case).

Thanks,
Giuliano.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141714: Fix ast print of variables with attributes

[Giuliano Belinassi via Phabricator via cfe-commits]

giulianobelinassi updated this revision to Diff 547477.
giulianobelinassi added a comment.
Herald added subscribers: wangpc, s.egerton, simoncook, asb.

- Use tblgen to generate table of attributes that can or must be print on the 
left side of the Decl.
- Use LLVM_MARK_AS_BITMASK_ENUM on AttrPrintLoc enum.
- Print attributes of declarations contatining parenthesis ctors on the left 
side. Improves the case:

  StructWithCopyConstructor s __attribute__((blocks("byref")))(5)

  now printing:

  __attribute__((blocks("byref"))) StructWithCopyConstructor s(5)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/CMakeLists.txt
  clang/lib/AST/DeclPrinter.cpp
  clang/test/AST/ast-print-attr-knr.c
  clang/test/AST/ast-print-attr.c
  clang/test/AST/ast-print-pragmas.cpp
  clang/test/Analysis/blocks.mm
  clang/test/OpenMP/assumes_codegen.cpp
  clang/test/OpenMP/assumes_print.cpp
  clang/test/OpenMP/assumes_template_print.cpp
  clang/test/OpenMP/declare_simd_ast_print.cpp
  clang/test/Sema/attr-print.c
  clang/test/SemaCXX/attr-print.cpp
  clang/test/SemaCXX/cxx11-attr-print.cpp
  clang/utils/TableGen/ClangAttrEmitter.cpp
  clang/utils/TableGen/TableGen.cpp
  clang/utils/TableGen/TableGenBackends.h

Index: clang/utils/TableGen/TableGenBackends.h
===
--- clang/utils/TableGen/TableGenBackends.h
+++ clang/utils/TableGen/TableGenBackends.h
@@ -39,6 +39,8 @@
 void EmitClangAttrClass(llvm::RecordKeeper , llvm::raw_ostream );
 void EmitClangAttrImpl(llvm::RecordKeeper , llvm::raw_ostream );
 void EmitClangAttrList(llvm::RecordKeeper , llvm::raw_ostream );
+void EmitClangAttrPrintList(const std::string ,
+llvm::RecordKeeper , llvm::raw_ostream );
 void EmitClangAttrSubjectMatchRuleList(llvm::RecordKeeper ,
llvm::raw_ostream );
 void EmitClangAttrPCHRead(llvm::RecordKeeper , llvm::raw_ostream );
Index: clang/utils/TableGen/TableGen.cpp
===
--- clang/utils/TableGen/TableGen.cpp
+++ clang/utils/TableGen/TableGen.cpp
@@ -31,6 +31,8 @@
   GenClangAttrSubjectMatchRulesParserStringSwitches,
   GenClangAttrImpl,
   GenClangAttrList,
+  GenClangAttrCanPrintLeftList,
+  GenClangAttrMustPrintLeftList,
   GenClangAttrDocTable,
   GenClangAttrSubjectMatchRuleList,
   GenClangAttrPCHRead,
@@ -127,6 +129,14 @@
"Generate clang attribute implementations"),
 clEnumValN(GenClangAttrList, "gen-clang-attr-list",
"Generate a clang attribute list"),
+clEnumValN(GenClangAttrCanPrintLeftList,
+   "gen-clang-attr-can-print-left-list",
+   "Generate list of attributes that can be printed on left "
+   "side of a decl"),
+clEnumValN(GenClangAttrMustPrintLeftList,
+   "gen-clang-attr-must-print-left-list",
+   "Generate list of attributes that must be printed on left "
+   "side of a decl"),
 clEnumValN(GenClangAttrDocTable, "gen-clang-attr-doc-table",
"Generate a table of attribute documentation"),
 clEnumValN(GenClangAttrSubjectMatchRuleList,
@@ -269,11 +279,14 @@
"Generate riscv_vector_builtin_cg.inc for clang"),
 clEnumValN(GenRISCVVectorBuiltinSema, "gen-riscv-vector-builtin-sema",
"Generate riscv_vector_builtin_sema.inc for clang"),
-clEnumValN(GenRISCVSiFiveVectorBuiltins, "gen-riscv-sifive-vector-builtins",
+clEnumValN(GenRISCVSiFiveVectorBuiltins,
+   "gen-riscv-sifive-vector-builtins",
"Generate riscv_sifive_vector_builtins.inc for clang"),
-clEnumValN(GenRISCVSiFiveVectorBuiltinCG, "gen-riscv-sifive-vector-builtin-codegen",
+clEnumValN(GenRISCVSiFiveVectorBuiltinCG,
+   "gen-riscv-sifive-vector-builtin-codegen",
"Generate riscv_sifive_vector_builtin_cg.inc for clang"),
-clEnumValN(GenRISCVSiFiveVectorBuiltinSema, "gen-riscv-sifive-vector-builtin-sema",
+clEnumValN(GenRISCVSiFiveVectorBuiltinSema,
+   "gen-riscv-sifive-vector-builtin-sema",
"Generate riscv_sifive_vector_builtin_sema.inc for clang"),
 clEnumValN(GenAttrDocs, "gen-attr-docs",
"Generate attribute documentation"),
@@ -315,6 +328,12 @@
   case GenClangAttrList:
 EmitClangAttrList(Records, OS);
 break;
+  case GenClangAttrCanPrintLeftList:
+EmitClangAttrPrintList("CanPrintOnLeftSide", Records, OS);
+break;
+  case GenClangAttrMustPrintLeftList:
+EmitClangAttrPrintList("MustPrintOnLeftSide", Records, OS);
+break;
   case GenClangAttrDocTable:
 EmitClangAttrDocTable(Records, OS);
 

[PATCH] D141714: Fix ast print of variables with attributes

[Giuliano Belinassi via Phabricator via cfe-commits]

giulianobelinassi marked 4 inline comments as done and 2 inline comments as 
done.
giulianobelinassi added a comment.

I will update the patch with the suggestions and send them again.




Comment at: clang/lib/AST/DeclPrinter.cpp:270
+
+  // FIXME: Find a way to use the AttrList.inc. We use if-else statements
+  // to classify each of them.

aaron.ballman wrote:
> erichkeane wrote:
> > I think this is something we need to just do the right way, right away.  
> > The below is completely unsustainable, and is just going to encourage us to 
> > spend the next few years messing with this if/else-if tree.  I'll leave 
> > final judgement to Aaron, but just making this its own function dependent 
> > on TableGen'ed files seems like what we should be doing from the start.
> As much as I hate to obligate anyone to get involved in tablegen to solve a 
> problem, I share the concern that this is not an extensible approach. I think 
> @giulianobelinassi should move forward by trying to emit this from 
> ClangAttrEmitter.cpp if at all possible.
I will do the TableGen approach, then..


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141714: Fix ast print of variables with attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/AST/DeclPrinter.cpp:270
+
+  // FIXME: Find a way to use the AttrList.inc. We use if-else statements
+  // to classify each of them.

erichkeane wrote:
> I think this is something we need to just do the right way, right away.  The 
> below is completely unsustainable, and is just going to encourage us to spend 
> the next few years messing with this if/else-if tree.  I'll leave final 
> judgement to Aaron, but just making this its own function dependent on 
> TableGen'ed files seems like what we should be doing from the start.
As much as I hate to obligate anyone to get involved in tablegen to solve a 
problem, I share the concern that this is not an extensible approach. I think 
@giulianobelinassi should move forward by trying to emit this from 
ClangAttrEmitter.cpp if at all possible.



Comment at: clang/test/Analysis/blocks.mm:81
 // ANALYZER-NEXT:   2: [B1.1] (CXXConstructExpr, [B1.3], 
StructWithCopyConstructor)
-// CHECK-NEXT:   3: StructWithCopyConstructor s(5) 
__attribute__((blocks("byref")));
+// CHECK-NEXT:   3: StructWithCopyConstructor s 
__attribute__((blocks("byref")))(5);
 // CHECK-NEXT:   4: ^{ }

erichkeane wrote:
> aaron.ballman wrote:
> > giulianobelinassi wrote:
> > > aaron.ballman wrote:
> > > > I can't quite tell if this change is good, bad, or just different.
> > > This indeed doesn't look good, but for what it is worth it is still 
> > > corretly accepted by clang and gcc.
> > I think this is a regression in terms of readability, but perhaps it's one 
> > we can live with.
> So I have a BIG concern here.  The primary purpose of AST print is to be 
> readable.  I don't think we should be willing to compromise that for what is, 
> to the project, a non-goal.
I think this issue would be resolved by going with a tablegen approach -- this 
attribute exists because of the `__block` keyword, so ideally, we wouldn't even 
be printing `__attribute__` here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141714: Fix ast print of variables with attributes

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/AST/DeclPrinter.cpp:270
+
+  // FIXME: Find a way to use the AttrList.inc. We use if-else statements
+  // to classify each of them.

I think this is something we need to just do the right way, right away.  The 
below is completely unsustainable, and is just going to encourage us to spend 
the next few years messing with this if/else-if tree.  I'll leave final 
judgement to Aaron, but just making this its own function dependent on 
TableGen'ed files seems like what we should be doing from the start.



Comment at: clang/test/Analysis/blocks.mm:81
 // ANALYZER-NEXT:   2: [B1.1] (CXXConstructExpr, [B1.3], 
StructWithCopyConstructor)
-// CHECK-NEXT:   3: StructWithCopyConstructor s(5) 
__attribute__((blocks("byref")));
+// CHECK-NEXT:   3: StructWithCopyConstructor s 
__attribute__((blocks("byref")))(5);
 // CHECK-NEXT:   4: ^{ }

aaron.ballman wrote:
> giulianobelinassi wrote:
> > aaron.ballman wrote:
> > > I can't quite tell if this change is good, bad, or just different.
> > This indeed doesn't look good, but for what it is worth it is still 
> > corretly accepted by clang and gcc.
> I think this is a regression in terms of readability, but perhaps it's one we 
> can live with.
So I have a BIG concern here.  The primary purpose of AST print is to be 
readable.  I don't think we should be willing to compromise that for what is, 
to the project, a non-goal.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141714: Fix ast print of variables with attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

It looks like several comments are left unaddressed, are you planning on making 
the suggested changes?




Comment at: clang/lib/AST/DeclPrinter.cpp:52-58
+enum AttrPrintLoc {
+  SIDE_NONE = 0,
+  SIDE_LEFT = 1,
+  SIDE_MIDDLE = 2,
+  SIDE_RIGHT = 4,
+  SIDE_ANY = SIDE_LEFT | SIDE_MIDDLE | SIDE_RIGHT,
+};

giulianobelinassi wrote:
> aaron.ballman wrote:
> > aaron.ballman wrote:
> > > 
> > I think we should use an `enum class` just so we don't steal these 
> > identifiers at the global scope within this file, WDYT?
> Unfortunately that would result in necessary auxiliary code to do an bitwise 
> '&' operation, so I don't think this is a good idea. Although I've explicitly 
> now access those constants by using the AttrPrintLoc:: keyword rather than 
> directly referencing the constant directly.
We have helper functionality for that, see `LLVM_MARK_AS_BITMASK_ENUM` from 
https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/ADT/BitmaskEnum.h




Comment at: clang/test/Analysis/blocks.mm:81
 // ANALYZER-NEXT:   2: [B1.1] (CXXConstructExpr, [B1.3], 
StructWithCopyConstructor)
-// CHECK-NEXT:   3: StructWithCopyConstructor s(5) 
__attribute__((blocks("byref")));
+// CHECK-NEXT:   3: StructWithCopyConstructor s 
__attribute__((blocks("byref")))(5);
 // CHECK-NEXT:   4: ^{ }

giulianobelinassi wrote:
> aaron.ballman wrote:
> > I can't quite tell if this change is good, bad, or just different.
> This indeed doesn't look good, but for what it is worth it is still corretly 
> accepted by clang and gcc.
I think this is a regression in terms of readability, but perhaps it's one we 
can live with.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141714: Fix ast print of variables with attributes

[Giuliano Belinassi via Phabricator via cfe-commits]

giulianobelinassi added a comment.

@aaron.ballman Ping


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141714: Fix ast print of variables with attributes

[Giuliano Belinassi via Phabricator via cfe-commits]

giulianobelinassi updated this revision to Diff 519131.
giulianobelinassi added a comment.

Incorporate some of Aron suggestions:

- Replace isDeclspecAttribute with isStandardAttributeSyntax
- Avoid multiple calls to getAsFunction
- Use AttrPrintLoc:: instead of referencing the value directly
- Also output enable_if on the right side of decl.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714

Files:
  clang/lib/AST/DeclPrinter.cpp
  clang/test/AST/ast-print-attr-knr.c
  clang/test/AST/ast-print-attr.c
  clang/test/AST/ast-print-pragmas.cpp
  clang/test/Analysis/blocks.mm
  clang/test/OpenMP/assumes_codegen.cpp
  clang/test/OpenMP/assumes_print.cpp
  clang/test/OpenMP/assumes_template_print.cpp
  clang/test/OpenMP/declare_simd_ast_print.cpp
  clang/test/Sema/attr-print.c
  clang/test/SemaCXX/attr-print.cpp
  clang/test/SemaCXX/cxx11-attr-print.cpp

Index: clang/test/SemaCXX/cxx11-attr-print.cpp
===
--- clang/test/SemaCXX/cxx11-attr-print.cpp
+++ clang/test/SemaCXX/cxx11-attr-print.cpp
@@ -3,8 +3,7 @@
 // CHECK: int x __attribute__((aligned(4)));
 int x __attribute__((aligned(4)));
 
-// FIXME: Print this at a valid location for a __declspec attr.
-// CHECK: int y __declspec(align(4));
+// CHECK: __declspec(align(4)) int y;
 __declspec(align(4)) int y;
 
 // CHECK: int z {{\[}}[gnu::aligned(4)]];
@@ -65,7 +64,7 @@
 
 // CHECK: int m __attribute__((aligned(4
 // CHECK: int n alignas(4
-// CHECK: static int f() __attribute__((pure))
+// CHECK: __attribute__((pure)) static int f()
 // CHECK: static int g() {{\[}}[gnu::pure]]
 template  struct S {
   __attribute__((aligned(4))) int m;
@@ -80,7 +79,7 @@
 
 // CHECK: int m __attribute__((aligned(4
 // CHECK: int n alignas(4
-// CHECK: static int f() __attribute__((pure))
+// CHECK: __attribute__((pure)) static int f()
 // CHECK: static int g() {{\[}}[gnu::pure]]
 template struct S;
 
Index: clang/test/SemaCXX/attr-print.cpp
===
--- clang/test/SemaCXX/attr-print.cpp
+++ clang/test/SemaCXX/attr-print.cpp
@@ -3,8 +3,7 @@
 // CHECK: int x __attribute__((aligned(4)));
 int x __attribute__((aligned(4)));
 
-// FIXME: Print this at a valid location for a __declspec attr.
-// CHECK: int y __declspec(align(4));
+// CHECK: __declspec(align(4)) int y;
 __declspec(align(4)) int y;
 
 // CHECK: void foo() __attribute__((const));
Index: clang/test/Sema/attr-print.c
===
--- clang/test/Sema/attr-print.c
+++ clang/test/Sema/attr-print.c
@@ -3,8 +3,7 @@
 // CHECK: int x __attribute__((aligned(4)));
 int x __attribute__((aligned(4)));
 
-// FIXME: Print this at a valid location for a __declspec attr.
-// CHECK: int y __declspec(align(4));
+// CHECK: __declspec(align(4)) int y;
 __declspec(align(4)) int y;
 
 // CHECK: short arr[3] __attribute__((aligned));
Index: clang/test/OpenMP/declare_simd_ast_print.cpp
===
--- clang/test/OpenMP/declare_simd_ast_print.cpp
+++ clang/test/OpenMP/declare_simd_ast_print.cpp
@@ -60,11 +60,11 @@
 
 class VV {
   // CHECK: #pragma omp declare simd uniform(this, a) linear(val(b): a)
-  // CHECK-NEXT: int add(int a, int b) __attribute__((cold)){
+  // CHECK-NEXT:  __attribute__((cold)) int add(int a, int b) {
   // CHECK-NEXT: return a + b;
   // CHECK-NEXT: }
   #pragma omp declare simd uniform(this, a) linear(val(b): a)
-  int add(int a, int b) __attribute__((cold)) { return a + b; }
+  __attribute__((cold)) int add(int a, int b) { return a + b; }
 
   // CHECK: #pragma omp declare simd aligned(b: 4) aligned(a) linear(ref(b): 4) linear(val(this)) linear(val(a))
   // CHECK-NEXT: float taddpf(float *a, float *) {
Index: clang/test/OpenMP/assumes_template_print.cpp
===
--- clang/test/OpenMP/assumes_template_print.cpp
+++ clang/test/OpenMP/assumes_template_print.cpp
@@ -17,7 +17,7 @@
 struct S {
   int a;
 // CHECK: template  struct S {
-// CHECK: void foo() __attribute__((assume("ompx_global_assumption"))) {
+// CHECK: __attribute__((assume("ompx_global_assumption"))) void foo() {
   void foo() {
 #pragma omp parallel
 {}
@@ -25,15 +25,15 @@
 };
 
 // CHECK: template<> struct S {
-// CHECK: void foo() __attribute__((assume("ompx_global_assumption"))) {
+// CHECK: __attribute__((assume("ompx_global_assumption"))) void foo() {
 
 #pragma omp begin assumes no_openmp
-// CHECK: void S_with_assumes_no_call() __attribute__((assume("omp_no_openmp"))) __attribute__((assume("ompx_global_assumption"))) {
+// CHECK: __attribute__((assume("omp_no_openmp"))) __attribute__((assume("ompx_global_assumption"))) void S_with_assumes_no_call() {
 void S_with_assumes_no_call() {
   S s;
   s.a = 0;
 }
-// CHECK: void 

[PATCH] D141714: Fix ast print of variables with attributes

[Giuliano Belinassi via Phabricator via cfe-commits]

giulianobelinassi added a comment.

Hi. See inline answers. I will send the new version in a few minutes.




Comment at: clang/lib/AST/DeclPrinter.cpp:52-58
+enum AttrPrintLoc {
+  SIDE_NONE = 0,
+  SIDE_LEFT = 1,
+  SIDE_MIDDLE = 2,
+  SIDE_RIGHT = 4,
+  SIDE_ANY = SIDE_LEFT | SIDE_MIDDLE | SIDE_RIGHT,
+};

aaron.ballman wrote:
> aaron.ballman wrote:
> > 
> I think we should use an `enum class` just so we don't steal these 
> identifiers at the global scope within this file, WDYT?
Unfortunately that would result in necessary auxiliary code to do an bitwise 
'&' operation, so I don't think this is a good idea. Although I've explicitly 
now access those constants by using the AttrPrintLoc:: keyword rather than 
directly referencing the constant directly.



Comment at: clang/lib/AST/DeclPrinter.cpp:272
+  // to classify each of them.
+  if (A->isCXX11Attribute()) {
+// C++11 onwards attributes can not be placed on left side. Output on

aaron.ballman wrote:
> This should also handle C2x attributes, so I'd use 
> `isStandardAttributeSyntax()` instead.
Done.



Comment at: clang/lib/AST/DeclPrinter.cpp:279
+attrloc = Right;
+  } else if (kind == attr::DiagnoseIf) {
+// DiagnoseIf should be print on the right side because it may refer to

aaron.ballman wrote:
> There are other attributes for which this is true as well, like `enable_if`, 
> thread safety annotations, etc.
I have expanded this to enable_if, as it is trivial. The thread safety 
annotations is not that trivial and seems to not trigger any test failure. So I 
think I will leave this to a next patch that properly parses the attributes to 
find references to symbols declared in function argument.



Comment at: clang/test/Analysis/blocks.mm:81
 // ANALYZER-NEXT:   2: [B1.1] (CXXConstructExpr, [B1.3], 
StructWithCopyConstructor)
-// CHECK-NEXT:   3: StructWithCopyConstructor s(5) 
__attribute__((blocks("byref")));
+// CHECK-NEXT:   3: StructWithCopyConstructor s 
__attribute__((blocks("byref")))(5);
 // CHECK-NEXT:   4: ^{ }

aaron.ballman wrote:
> I can't quite tell if this change is good, bad, or just different.
This indeed doesn't look good, but for what it is worth it is still corretly 
accepted by clang and gcc.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141714: Fix ast print of variables with attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/AST/DeclPrinter.cpp:52-58
+enum AttrPrintLoc {
+  SIDE_NONE = 0,
+  SIDE_LEFT = 1,
+  SIDE_MIDDLE = 2,
+  SIDE_RIGHT = 4,
+  SIDE_ANY = SIDE_LEFT | SIDE_MIDDLE | SIDE_RIGHT,
+};

aaron.ballman wrote:
> 
I think we should use an `enum class` just so we don't steal these identifiers 
at the global scope within this file, WDYT?



Comment at: clang/lib/AST/DeclPrinter.cpp:264-266
+  // 1- should be print on the left side of a decl.
+  // 2- should be print on the middle of a decl right after the type.
+  // 3- should be print on the right side of a decl.





Comment at: clang/lib/AST/DeclPrinter.cpp:267-268
+  // 3- should be print on the right side of a decl.
+  AttrPrintLoc attrloc = Right; // We default to middle.
+  attr::Kind kind = A->getKind();
+

Minor style nits.



Comment at: clang/lib/AST/DeclPrinter.cpp:272
+  // to classify each of them.
+  if (A->isCXX11Attribute()) {
+// C++11 onwards attributes can not be placed on left side. Output on

This should also handle C2x attributes, so I'd use 
`isStandardAttributeSyntax()` instead.



Comment at: clang/lib/AST/DeclPrinter.cpp:279
+attrloc = Right;
+  } else if (kind == attr::DiagnoseIf) {
+// DiagnoseIf should be print on the right side because it may refer to

There are other attributes for which this is true as well, like `enable_if`, 
thread safety annotations, etc.



Comment at: clang/lib/AST/DeclPrinter.cpp:286-287
+attrloc = Left;
+  } else if (D->getAsFunction() &&
+ D->getAsFunction()->isThisDeclarationADefinition()) {
+// In case Decl is a function with a body, then attrs should be print





Comment at: clang/lib/AST/DeclPrinter.cpp:664-665
+
+  std::string Proto;
+  llvm::raw_string_ostream OS(Proto);
+

What's the purpose to hoisting these two lines?



Comment at: clang/test/Analysis/blocks.mm:81
 // ANALYZER-NEXT:   2: [B1.1] (CXXConstructExpr, [B1.3], 
StructWithCopyConstructor)
-// CHECK-NEXT:   3: StructWithCopyConstructor s(5) 
__attribute__((blocks("byref")));
+// CHECK-NEXT:   3: StructWithCopyConstructor s 
__attribute__((blocks("byref")))(5);
 // CHECK-NEXT:   4: ^{ }

I can't quite tell if this change is good, bad, or just different.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141714: Fix ast print of variables with attributes

[Giuliano Belinassi via Phabricator via cfe-commits]

giulianobelinassi updated this revision to Diff 514473.
giulianobelinassi added a comment.
Herald added a reviewer: jdoerfert.
Herald added subscribers: jplehr, sstefan1.

Update patch with the agreements  of last discussion.

This new version includes code to handle the cases presented by Aron,
which are:

  1- Variable with attributes have its attribute dumped before its value.
  
  2- __declspec attributes are dumped on the left side of the declaration,
 as recommended by MSVC docs.
  
  3- Functions with body has its attributes dumped on the right side of
 the declaration instead of left side when possible.  The point of
 this is to (1) avoid attribute placement confusion in K C
 functions and (2) keep compatibility with GCC. The case where
 it is possible to have a variable referenced in the attribute is
 also handled specially (AttributeIf).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714

Files:
  clang/lib/AST/DeclPrinter.cpp
  clang/test/AST/ast-print-attr-knr.c
  clang/test/AST/ast-print-attr.c
  clang/test/AST/ast-print-pragmas.cpp
  clang/test/Analysis/blocks.mm
  clang/test/OpenMP/assumes_codegen.cpp
  clang/test/OpenMP/assumes_print.cpp
  clang/test/OpenMP/assumes_template_print.cpp
  clang/test/OpenMP/declare_simd_ast_print.cpp
  clang/test/Sema/attr-print.c
  clang/test/SemaCXX/attr-print.cpp
  clang/test/SemaCXX/cxx11-attr-print.cpp

Index: clang/test/SemaCXX/cxx11-attr-print.cpp
===
--- clang/test/SemaCXX/cxx11-attr-print.cpp
+++ clang/test/SemaCXX/cxx11-attr-print.cpp
@@ -3,8 +3,7 @@
 // CHECK: int x __attribute__((aligned(4)));
 int x __attribute__((aligned(4)));
 
-// FIXME: Print this at a valid location for a __declspec attr.
-// CHECK: int y __declspec(align(4));
+// CHECK: __declspec(align(4)) int y;
 __declspec(align(4)) int y;
 
 // CHECK: int z {{\[}}[gnu::aligned(4)]];
@@ -65,7 +64,7 @@
 
 // CHECK: int m __attribute__((aligned(4
 // CHECK: int n alignas(4
-// CHECK: static int f() __attribute__((pure))
+// CHECK: __attribute__((pure)) static int f()
 // CHECK: static int g() {{\[}}[gnu::pure]]
 template  struct S {
   __attribute__((aligned(4))) int m;
@@ -80,7 +79,7 @@
 
 // CHECK: int m __attribute__((aligned(4
 // CHECK: int n alignas(4
-// CHECK: static int f() __attribute__((pure))
+// CHECK: __attribute__((pure)) static int f()
 // CHECK: static int g() {{\[}}[gnu::pure]]
 template struct S;
 
Index: clang/test/SemaCXX/attr-print.cpp
===
--- clang/test/SemaCXX/attr-print.cpp
+++ clang/test/SemaCXX/attr-print.cpp
@@ -3,8 +3,7 @@
 // CHECK: int x __attribute__((aligned(4)));
 int x __attribute__((aligned(4)));
 
-// FIXME: Print this at a valid location for a __declspec attr.
-// CHECK: int y __declspec(align(4));
+// CHECK: __declspec(align(4)) int y;
 __declspec(align(4)) int y;
 
 // CHECK: void foo() __attribute__((const));
Index: clang/test/Sema/attr-print.c
===
--- clang/test/Sema/attr-print.c
+++ clang/test/Sema/attr-print.c
@@ -3,8 +3,7 @@
 // CHECK: int x __attribute__((aligned(4)));
 int x __attribute__((aligned(4)));
 
-// FIXME: Print this at a valid location for a __declspec attr.
-// CHECK: int y __declspec(align(4));
+// CHECK: __declspec(align(4)) int y;
 __declspec(align(4)) int y;
 
 // CHECK: short arr[3] __attribute__((aligned));
Index: clang/test/OpenMP/declare_simd_ast_print.cpp
===
--- clang/test/OpenMP/declare_simd_ast_print.cpp
+++ clang/test/OpenMP/declare_simd_ast_print.cpp
@@ -60,11 +60,11 @@
 
 class VV {
   // CHECK: #pragma omp declare simd uniform(this, a) linear(val(b): a)
-  // CHECK-NEXT: int add(int a, int b) __attribute__((cold)){
+  // CHECK-NEXT:  __attribute__((cold)) int add(int a, int b) {
   // CHECK-NEXT: return a + b;
   // CHECK-NEXT: }
   #pragma omp declare simd uniform(this, a) linear(val(b): a)
-  int add(int a, int b) __attribute__((cold)) { return a + b; }
+  __attribute__((cold)) int add(int a, int b) { return a + b; }
 
   // CHECK: #pragma omp declare simd aligned(b: 4) aligned(a) linear(ref(b): 4) linear(val(this)) linear(val(a))
   // CHECK-NEXT: float taddpf(float *a, float *) {
Index: clang/test/OpenMP/assumes_template_print.cpp
===
--- clang/test/OpenMP/assumes_template_print.cpp
+++ clang/test/OpenMP/assumes_template_print.cpp
@@ -17,7 +17,7 @@
 struct S {
   int a;
 // CHECK: template  struct S {
-// CHECK: void foo() __attribute__((assume("ompx_global_assumption"))) {
+// CHECK: __attribute__((assume("ompx_global_assumption"))) void foo() {
   void foo() {
 #pragma omp parallel
 {}
@@ -25,15 +25,15 @@
 };
 
 // CHECK: template<> struct S {
-// 

[PATCH] D141714: Fix ast print of variables with attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D141714#4175942 , 
@giulianobelinassi wrote:

> Hi, Aron.
>
> Just to make myself clear: What I need to do is that the clang dumps for C 
> files are also accepted by GCC as input.

FWIW, that's not a supported use for `-ast-print`. We've never had a 
requirement that `-ast-print` be useful for anything more than debugging 
scenarios and so the printing functionality *frequently* produces incorrect 
output, especially on newer language constructs. We fix up the functionality in 
an ad hoc manner as people find a reason to care about a particular situation.

If your goal is so that `-ast-print` reliably produces compilable code, I think 
that requires buy-in from the Clang community because that's a bit of a heavy 
lift for the community to support. Traditionally, we've pointed users to other 
tools that are usually better-suited to the task trying to be solved (like 
`clang-format` for pretty printing or stencils for performing source to source 
transformations, etc).

> Here is why I wanted to output the attribute on middle:
>
> https://godbolt.org/z/6aPc6aWcz
>
> As you can see, GCC complains of `__attributes__` output on the right side of 
> functions with bodies.
>
> But overall, perhaps what should be output in the dumps are the following 
> (please check if you agree with me):
>
> 1- Attributes in variables should be output to the right side, as it was done 
> before. That is:
>
>   int var __attribute__((unused)) = 0;

Agreed. FWIW, I don't mind if that winds up producing:

  int var1 __attribute__((unused)) = 0, var2 __attribute__((unused)) = 0;

instead of:

  __attribute__((unused)) int var1 = 0, var2 = 0;



> 2- Variables or functions declared with __declspec attributes should go to 
> the left side, as does MSVC says is recommended. That is:
>
>   __declspec(thread) int var = 0;
>   __declspec(noinline) int f(void);
>   __declspec(noinline) int f(void) { return 0; }

Agreed.

> 3- Functions __prototypes__ should have its attributes output to the right 
> side, that means:
>
>   int f(void) __attribute__((unused));

So long as this doesn't change program meaning for any attributes, that seems 
reasonable enough.

> 4- But attributes specified in function declaration __with body__ should go 
> to the __left__ side __because GCC rejects outputing them on the right 
> side__, as:
>
>   __attribute__((unused)) int f(void) { return 0; }

This will break code for folks compiling with Clang, though, so it has to be 
done on a case-by-case basis. Any attribute accepting an expression argument 
that appears on the right of the function needs to continue to appear to the 
right of the function the expression refers to a parameter name. The same is 
true for use with lambdas.

> -
>
> The result of this choice would be that the following K function __as 
> input__ (notice how it is not clear where the __attribute__ is being applied 
> to:
>
>   int f(i)
>__attribute__((unused)) int i;
>   { return 0; }
>
> would be dumped as:
>
>   __attribute__((unused)) int f(i)
>   int i;
>   { return 0; }
>
> But in practical terms, GCC would accept it without problems and it is clear 
> where the __attribute__ is being applied to. Outputting to the right side has 
> some ambiguity here, which is what I want to avoid.

Again, this only works for some attributes. Consider: 
https://godbolt.org/z/3YreGY1G4

There's another case to think about, which are tag types: 
https://godbolt.org/z/KjKn7rz89

And finally, there's `[[]]` attributes where changing the position from what's 
in source will potentially change what the attribute appertains to and also 
break code. So I think to achieve the goal you're after, there's actually quite 
a bit of work to be done. However, so long as the output doesn't get *worse* 
for other situations, I think any incremental progress here is acceptable. 
e.g., fixing this kind of nonsense is a good incremental step even if we do 
nothing else:

  // MS-EXT-NEXT: int x = 3 __declspec(thread);
  int __declspec(thread) x = 3;


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141714: Fix ast print of variables with attributes

[Giuliano Belinassi via Phabricator via cfe-commits]

giulianobelinassi added a comment.

Hi, Aron.

Just to make myself clear: What I need to do is that the clang dumps for C 
files are also accepted by GCC as input.

Here is why I wanted to output the attribute on middle:

https://godbolt.org/z/6aPc6aWcz

As you can see, GCC complains of `__attributes__` output on the right side of 
functions with bodies.

But overall, perhaps what should be output in the dumps are the following 
(please check if you agree with me):

1- Attributes in variables should be output to the right side, as it was done 
before. That is:

  int var __attribute__((unused)) = 0;

2- Variables or functions declared with __declspec attributes should go to the 
left side, as does MSVC says is recommended. That is:

  __declspec(thread) int var = 0;
  __declspec(noinline) int f(void);
  __declspec(noinline) int f(void) { return 0; }

3- Functions __prototypes__ should have its attributes output to the right 
side, that means:

  int f(void) __attribute__((unused));

4- But attributes specified in function declaration __with body__ should go to 
the __left__ side __because GCC rejects outputing them on the right side__, as:

  __attribute__((unused)) int f(void) { return 0; }

-

The result of this choice would be that the following K function __as input__ 
(notice how it is not clear where the __attribute__ is being applied to:

  int f(i)
   __attribute__((unused)) int i;
  { return 0; }

would be dumped as:

  __attribute__((unused)) int f(i)
  int i;
  { return 0; }

But in practical terms, GCC would accept it without problems and it is clear 
where the __attribute__ is being applied to. Outputting to the right side has 
some ambiguity here, which is what I want to avoid.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141714: Fix ast print of variables with attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D141714#4175263 , 
@giulianobelinassi wrote:

> Hi, Alan. Thanks for your review again!
>
> With regard to middle, the patch sent in D145269 
>  may answer your questions. Basically 
> functions like:
>
>   int* f(void) __attribute__((unused));
>
> would be output as
>
>   int* __attribute__((unused)) f(void);
>
> if SIDE_MIDDLE is specified.

That would be a pretty unique location for those attributes to go and there are 
some attributes that can't go there. For example: 
https://godbolt.org/z/fo5cnEGTY

> The case I want to avoid is:
>
>   int f(void) __attribute__((unused))
> int i;
>   { return 0; }

Did you mean the example to be a K C function instead? If so, then this would 
apply the attribute to the function in Clang.

> Which is not clear where should the attribute be applied to. A left-to-right 
> parser that accepts attributes on the right side of a function declaration 
> may apply this to f, but if it don't then it will be applied to i. This is 
> why GCC rejects this kind of attribute output on the right side of functions.
> Hence, I thought it would be better to always output variable as
>
>   int __attribute__((unused)) var;

That will subtly change program semantics in Clang though, as now the attribute 
pretty prints to the parameter declaration (I'm still assuming your example was 
intended to be a K C function definition).

> when possible. I also found that __declspec attributes is also accepted this 
> way. But if that changes the meaning of things (I looked into generated 
> assembly and could not find such case) then I think dumping it on the right 
> side may be a better option, and we only have to be careful with functions 
> declarations, those being dumped as.
>
>   __attribute__((unused)) f(void) {}
>
> -
>
> As for changing the `__declspec` locations, I thought it was fine as it is 
> also accepted by clang. But looking at MSVC documentation it says that 
> outputing it at the begining is recommended so I think it may be better to 
> always output __declspecs on the left side?

That's where I'm most used to seeing it.

> -
>
> Can you explain a bit more about what warning you're seeing and under what 
> condition?
>
> Basically if you change this test in order for it to intentionally fail 
> because of output mismatch, this warning is generated.

Hmmm, I'm still not certain I understand what's going on. What surprises me is 
that the test code is ` __block StructWithCopyConstructor s(5);` but it pretty 
prints as `StructWithCopyConstructor __attribute__((blocks("byref"))) s(5);` 
which doesn't compile at all. That doesn't seem to be new behavior in your 
patch (and this particular attribute has no documentation or existing test 
coverage), but I think the fact that the keyword spelling is dropped and the 
attribute is moved helps point out an issue -- keyword attributes are special 
and their syntactic position is important, so maybe those should not shift at 
all.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141714: Fix ast print of variables with attributes

[Giuliano Belinassi via Phabricator via cfe-commits]

giulianobelinassi added a comment.

Hi, Alan. Thanks for your review again!

With regard to middle, the patch sent in D145269 
 may answer your questions. Basically 
functions like:

  int* f(void) __attribute__((unused));

would be output as

  int* __attribute__((unused)) f(void);

if SIDE_MIDDLE is specified.

The case I want to avoid is:

  int f(void) __attribute__((unused))
int i;
  { return 0; }

Which is not clear where should the attribute be applied to. A left-to-right 
parser that accepts attributes on the right side of a function declaration may 
apply this to f, but if it don't then it will be applied to i. This is why GCC 
rejects this kind of attribute output on the right side of functions.
Hence, I thought it would be better to always output variable as

  int __attribute__((unused)) var;

when possible. I also found that __declspec attributes is also accepted this 
way. But if that changes the meaning of things (I looked into generated 
assembly and could not find such case) then I think dumping it on the right 
side may be a better option, and we only have to be careful with functions 
declarations, those being dumped as.

  __attribute__((unused)) f(void) {}

-

As for changing the `__declspec` locations, I thought it was fine as it is also 
accepted by clang. But looking at MSVC documentation it says that outputing it 
at the begining is recommended so I think it may be better to always output 
__declspecs on the left side?

-

Can you explain a bit more about what warning you're seeing and under what 
condition?

Basically if you change this test in order for it to intentionally fail because 
of output mismatch, this warning is generated.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141714: Fix ast print of variables with attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: erichkeane.
aaron.ballman added inline comments.



Comment at: clang/lib/AST/DeclPrinter.cpp:52-58
+enum AttrPrintLoc {
+  SIDE_NONE = 0,
+  SIDE_LEFT = 1,
+  SIDE_MIDDLE = 2,
+  SIDE_RIGHT = 4,
+  SIDE_ANY = SIDE_LEFT | SIDE_MIDDLE | SIDE_RIGHT,
+};





Comment at: clang/lib/AST/DeclPrinter.cpp:60-61
+
+void prettyPrintAttributes(Decl *D, raw_ostream ,
+   AttrPrintLoc loc = SIDE_ANY);
+





Comment at: clang/lib/AST/DeclPrinter.cpp:252-253
 
-void DeclPrinter::prettyPrintAttributes(Decl *D) {
+void DeclPrinter::prettyPrintAttributes(Decl *D, llvm::raw_ostream ,
+AttrPrintLoc loc) {
   if (Policy.PolishForDeclaration)





Comment at: clang/lib/AST/DeclPrinter.cpp:267
+  // 3- should be print on the right side of a decl.
+  AttrPrintLoc attrloc = SIDE_MIDDLE; // We default to middle.
+  attr::Kind kind = A->getKind();

FWIW, I'm more used to seeing `__declspec` and `__attribute__` leading the 
declaration specifiers instead of trailing them. Also, "middle" raises 
questions for me as to where the attribute will go for a function. Given:
```
__attribute__((foo)) void bar(void);
```
what happens? All the test coverage that's changed currently is for variables, 
not for functions, so I can't really tell.

Also, this should be tested with keyword attributes like `alignas` and 
`__global` because those have very specific parse orders.



Comment at: clang/lib/AST/DeclPrinter.cpp:273-274
+  if (A->isCXX11Attribute()) {
+// C++11 onwards attributes can not be placed on middle. Output on
+// right to mimic old clang behaviour.
+attrloc = SIDE_RIGHT;

This comment isn't quite accurate -- it's more that `[[]]` attributes have very 
specific rules about what they appertain to based on the syntactic location of 
the `[[]]`. e.g.,
```
[[foo]] int a, b; // foo applies to both a and b
int [[foo]] a, b; // foo applies to the type of int, which forms the type for 
both a and b
int a [[foo]], b; // foo applies to just a
```
Also, the same logic applies to `[[]]` in C as well as C++, so I think you 
meant to use `A->isStandardAttributeSyntax()`.



Comment at: clang/test/Analysis/blocks.mm:73-74
 
+// FIXME: C++ issues a ignore warning on this __attribute__ output.
+
 // CHECK-LABEL:void testBlockWithCaptureByReference()

Can you explain a bit more about what warning you're seeing and under what 
condition?



Comment at: clang/test/Sema/attr-print.c:4
-// CHECK: int x __attribute__((aligned(4)));
-int x __attribute__((aligned(4)));
 

Why did you change where the attribute is written in this test?



Comment at: clang/test/Sema/attr-print.c:8-11
-__declspec(align(4)) int y;
+// CHECK: int __declspec(align(4)) y;
+int __declspec(align(4)) y;
 
-// CHECK: short arr[3] __attribute__((aligned));
-short arr[3] __attribute__((aligned));

Same questions here.



Comment at: clang/test/SemaCXX/attr-print.cpp:3-4
 
-// CHECK: int x __attribute__((aligned(4)));
-int x __attribute__((aligned(4)));
 

Same questions here as above.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141714: Fix ast print of variables with attributes

[Giuliano Belinassi via Phabricator via cfe-commits]

giulianobelinassi updated this revision to Diff 501835.
giulianobelinassi edited the summary of this revision.
giulianobelinassi added a comment.

Update to dump attributes right after the type specification, also
fixing the __declspec case.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714

Files:
  clang/lib/AST/DeclPrinter.cpp
  clang/test/AST/ast-print-attr-knr.c
  clang/test/AST/ast-print-attr.c
  clang/test/AST/ast-print-pragmas.cpp
  clang/test/Analysis/blocks.mm
  clang/test/Sema/attr-print.c
  clang/test/SemaCXX/attr-print.cpp
  clang/test/SemaCXX/cxx11-attr-print.cpp

Index: clang/test/SemaCXX/cxx11-attr-print.cpp
===
--- clang/test/SemaCXX/cxx11-attr-print.cpp
+++ clang/test/SemaCXX/cxx11-attr-print.cpp
@@ -1,29 +1,28 @@
 // RUN: %clang_cc1 -std=c++11 -ast-print -fms-extensions %s | FileCheck %s
 //
-// CHECK: int x __attribute__((aligned(4)));
-int x __attribute__((aligned(4)));
+// CHECK: int __attribute__((aligned(4))) x;
+int __attribute__((aligned(4))) x;
 
-// FIXME: Print this at a valid location for a __declspec attr.
-// CHECK: int y __declspec(align(4));
-__declspec(align(4)) int y;
+// CHECK: int __declspec(align(4)) y;
+int __declspec(align(4)) y;
 
 // CHECK: int z {{\[}}[gnu::aligned(4)]];
 int z [[gnu::aligned(4)]];
 
-// CHECK: __attribute__((deprecated("warning")));
-int a __attribute__((deprecated("warning")));
+// CHECK: int __attribute__((deprecated("warning"))) a;
+int __attribute__((deprecated("warning"))) a;
 
 // CHECK: int b {{\[}}[gnu::deprecated("warning")]];
 int b [[gnu::deprecated("warning")]];
 
-// CHECK: __declspec(deprecated("warning"))
-__declspec(deprecated("warning")) int c;
+// CHECK: int __declspec(deprecated("warning")) c;
+int __declspec(deprecated("warning")) c;
 
 // CHECK: int d {{\[}}[deprecated("warning")]];
 int d [[deprecated("warning")]];
 
-// CHECK: __attribute__((deprecated("warning", "fixit")));
-int e __attribute__((deprecated("warning", "fixit")));
+// CHECK: int __attribute__((deprecated("warning", "fixit"))) e;
+int __attribute__((deprecated("warning", "fixit"))) e;
 
 // CHECK: int cxx11_alignas alignas(4);
 alignas(4) int cxx11_alignas;
@@ -63,12 +62,12 @@
 // CHECK: __attribute__((format(printf, 2, 3)));
 void f8 (void *, const char *, ...) __attribute__ ((format (printf, 2, 3)));
 
-// CHECK: int m __attribute__((aligned(4
+// CHECK: int m __attribute__((aligned(4)))
 // CHECK: int n alignas(4
 // CHECK: static int f() __attribute__((pure))
 // CHECK: static int g() {{\[}}[gnu::pure]]
 template  struct S {
-  __attribute__((aligned(4))) int m;
+  int __attribute__((aligned(4))) m;
   alignas(4) int n;
   __attribute__((pure)) static int f() {
 return 0;
@@ -78,7 +77,7 @@
   }
 };
 
-// CHECK: int m __attribute__((aligned(4
+// CHECK: int m __attribute__((aligned(4)))
 // CHECK: int n alignas(4
 // CHECK: static int f() __attribute__((pure))
 // CHECK: static int g() {{\[}}[gnu::pure]]
Index: clang/test/SemaCXX/attr-print.cpp
===
--- clang/test/SemaCXX/attr-print.cpp
+++ clang/test/SemaCXX/attr-print.cpp
@@ -1,11 +1,11 @@
 // RUN: %clang_cc1 %s -ast-print -fms-extensions | FileCheck %s
 
-// CHECK: int x __attribute__((aligned(4)));
-int x __attribute__((aligned(4)));
+// CHECK: int __attribute__((aligned(4))) x;
+int __attribute__((aligned(4))) x;
 
 // FIXME: Print this at a valid location for a __declspec attr.
-// CHECK: int y __declspec(align(4));
-__declspec(align(4)) int y;
+// CHECK: int __declspec(align(4)) y;
+int __declspec(align(4)) y;
 
 // CHECK: void foo() __attribute__((const));
 void foo() __attribute__((const));
@@ -20,11 +20,11 @@
 // CHECK: typedef int Small1 __attribute__((mode(byte)));
 typedef int Small1 __attribute__((mode(byte)));
 
-// CHECK: int small __attribute__((mode(byte)));
-int small __attribute__((mode(byte)));
+// CHECK: int __attribute__((mode(byte))) small;
+int __attribute__((mode(byte))) small;
 
-// CHECK: int v __attribute__((visibility("hidden")));
-int v __attribute__((visibility("hidden")));
+// CHECK: int __attribute__((visibility("hidden"))) v;
+int __attribute__((visibility("hidden"))) v;
 
 // CHECK: char *PR24565() __attribute__((malloc))
 char *PR24565() __attribute__((__malloc__));
Index: clang/test/Sema/attr-print.c
===
--- clang/test/Sema/attr-print.c
+++ clang/test/Sema/attr-print.c
@@ -1,14 +1,14 @@
 // RUN: %clang_cc1 %s -ast-print -fms-extensions | FileCheck %s
 
-// CHECK: int x __attribute__((aligned(4)));
-int x __attribute__((aligned(4)));
+// CHECK: int __attribute__((aligned(4))) x;
+int __attribute__((aligned(4))) x;
 
 // FIXME: Print this at a valid location for a __declspec attr.
-// CHECK: int y __declspec(align(4));
-__declspec(align(4)) int y;
+// CHECK: int __declspec(align(4)) y;

[PATCH] D141714: Fix ast print of variables with attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D141714#4077631 , 
@giulianobelinassi wrote:

>> The way Clang handles this is to mostly go back to the original source code 
>> (through the source manager and source location information) to grab what 
>> the user actually wrote. The pretty printing functionality loses information 
>> like whether something was expanded from a macro, etc and so, if the goal is 
>> to show what the user wrote, it's likely to be a really big uphill battle to 
>> get that through the pretty printer.
>
> This is something I already do, but there are some cases where I simply can 
> not track down the origin of the declaration (some very complex macro 
> expansions) and in this case I simply fall back to the prettyprinter. Those 
> are the cases where I am interesting in fixing the prettyprinter. It is also 
> worth noting that I am currently interested in C code so perhaps most of 
> these issues don't manifest in this case.

Ah, good to know; that should hopefully make it easier for you.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141714: Fix ast print of variables with attributes

[Giuliano Belinassi via Phabricator via cfe-commits]

giulianobelinassi added a comment.



> The way Clang handles this is to mostly go back to the original source code 
> (through the source manager and source location information) to grab what the 
> user actually wrote. The pretty printing functionality loses information like 
> whether something was expanded from a macro, etc and so, if the goal is to 
> show what the user wrote, it's likely to be a really big uphill battle to get 
> that through the pretty printer.

This is something I already do, but there are some cases where I simply can not 
track down the origin of the declaration (some very complex macro expansions) 
and in this case I simply fall back to the prettyprinter. Those are the cases 
where I am interesting in fixing the prettyprinter. It is also worth noting 
that I am currently interested in C code so perhaps most of these issues don't 
manifest in this case.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141714: Fix ast print of variables with attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D141714#4077207 , 
@giulianobelinassi wrote:

> In D141714#4077204 , @aaron.ballman 
> wrote:
>
>> In D141714#4077199 , 
>> @giulianobelinassi wrote:
>>
>>> In D141714#4077150 , 
>>> @aaron.ballman wrote:
>>>
 Thank you for the fix!

 It looks like precommit CI found a related failure that needs to be 
 addressed: 
 https://buildkite.com/llvm-project/premerge-checks/builds/130589#0185ac99-1158-46b3-b6d1-52fcf5310a59

 Can you also add a release note about the fix as well?
>>>
>>> Thank you for your review!
>>>
>>> I will update this patch once I get spare cycles to this. There is also an 
>>> additonal case regarding __declspec and K functions that needs to be 
>>> addressed as well in this patch which it currently doesn't do.
>>
>> You don't have to worry about the additional cases (unless you want to, but 
>> then they can be handled in separate patches); ast pretty printing is wrong, 
>> broken, and totally incorrect in a whole lot of places; we maintain it as a 
>> best effort as a debugging aid.
>
> That is interesting. I am developing a static analyzer which relies on this 
> for outputing code, so I would need those issues to be fixed for that project 
> to succeed. If you have additional cases already mapped and you want to share 
> with me I will happily fix them as well.

The way Clang handles this is to mostly go back to the original source code 
(through the source manager and source location information) to grab what the 
user actually wrote. The pretty printing functionality loses information like 
whether something was expanded from a macro, etc and so, if the goal is to show 
what the user wrote, it's likely to be a really big uphill battle to get that 
through the pretty printer. (The other issue is that we don't actively maintain 
the pretty printer, so it's quite frequently the case that newer language 
constructs are not well-supported by the pretty printer unless it just so 
happens to work by default.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141714: Fix ast print of variables with attributes

[Giuliano Belinassi via Phabricator via cfe-commits]

giulianobelinassi added a comment.

In D141714#4077204 , @aaron.ballman 
wrote:

> In D141714#4077199 , 
> @giulianobelinassi wrote:
>
>> In D141714#4077150 , 
>> @aaron.ballman wrote:
>>
>>> Thank you for the fix!
>>>
>>> It looks like precommit CI found a related failure that needs to be 
>>> addressed: 
>>> https://buildkite.com/llvm-project/premerge-checks/builds/130589#0185ac99-1158-46b3-b6d1-52fcf5310a59
>>>
>>> Can you also add a release note about the fix as well?
>>
>> Thank you for your review!
>>
>> I will update this patch once I get spare cycles to this. There is also an 
>> additonal case regarding __declspec and K functions that needs to be 
>> addressed as well in this patch which it currently doesn't do.
>
> You don't have to worry about the additional cases (unless you want to, but 
> then they can be handled in separate patches); ast pretty printing is wrong, 
> broken, and totally incorrect in a whole lot of places; we maintain it as a 
> best effort as a debugging aid.

That is interesting. I am developing a static analyzer which relies on this for 
outputing code, so I would need those issues to be fixed for that project to 
succeed. If you have additional cases already mapped and you want to share with 
me I will happily fix them as well.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141714: Fix ast print of variables with attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D141714#4077199 , 
@giulianobelinassi wrote:

> In D141714#4077150 , @aaron.ballman 
> wrote:
>
>> Thank you for the fix!
>>
>> It looks like precommit CI found a related failure that needs to be 
>> addressed: 
>> https://buildkite.com/llvm-project/premerge-checks/builds/130589#0185ac99-1158-46b3-b6d1-52fcf5310a59
>>
>> Can you also add a release note about the fix as well?
>
> Thank you for your review!
>
> I will update this patch once I get spare cycles to this. There is also an 
> additonal case regarding __declspec and K functions that needs to be 
> addressed as well in this patch which it currently doesn't do.

You don't have to worry about the additional cases (unless you want to, but 
then they can be handled in separate patches); ast pretty printing is wrong, 
broken, and totally incorrect in a whole lot of places; we maintain it as a 
best effort as a debugging aid.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141714: Fix ast print of variables with attributes

[Giuliano Belinassi via Phabricator via cfe-commits]

giulianobelinassi added a comment.

In D141714#4077150 , @aaron.ballman 
wrote:

> Thank you for the fix!
>
> It looks like precommit CI found a related failure that needs to be 
> addressed: 
> https://buildkite.com/llvm-project/premerge-checks/builds/130589#0185ac99-1158-46b3-b6d1-52fcf5310a59
>
> Can you also add a release note about the fix as well?

Thank you for your review!

I will update this patch once I get spare cycles to this. There is also an 
additonal case regarding __declspec and K functions that needs to be 
addressed as well in this patch which it currently doesn't do.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141714: Fix ast print of variables with attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: aaron.ballman.
aaron.ballman added a comment.

Thank you for the fix!

It looks like precommit CI found a related failure that needs to be addressed: 
https://buildkite.com/llvm-project/premerge-checks/builds/130589#0185ac99-1158-46b3-b6d1-52fcf5310a59

Can you also add a release note about the fix as well?




Comment at: clang/lib/AST/DeclPrinter.cpp:894
: D->getName());
+  prettyPrintAttributes(D);
   Expr *Init = D->getInit();

Adding in a comment for the problems that remain.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141714: Fix ast print of variables with attributes

[Giuliano Belinassi via Phabricator via cfe-commits]

giulianobelinassi created this revision.
Herald added a project: All.
giulianobelinassi requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Previously clang AST prints the following declaration:

int fun_var_unused() {

  int x __attribute__((unused)) = 0;
  return x;

}

as:

int fun_var_unused() {

  int x = 0 __attribute__((unused));
  return x;

}

which is rejected by C/C++ parser. This patch modifies the logic to
print as the first example.

Fixes: https://github.com/llvm/llvm-project/issues/59973

Signed-off-by: Giuliano Belinassi 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D141714

Files:
  clang/lib/AST/DeclPrinter.cpp
  clang/test/AST/ast-print-attr.c
  clang/test/AST/ast-print-pragmas.cpp


Index: clang/test/AST/ast-print-pragmas.cpp
===
--- clang/test/AST/ast-print-pragmas.cpp
+++ clang/test/AST/ast-print-pragmas.cpp
@@ -92,8 +92,9 @@
 
 #ifdef MS_EXT
 #pragma init_seg(compiler)
+/* FIXME: Trying to compile the output source results in compilation error.  */
 // MS-EXT: #pragma init_seg (.CRT$XCC){{$}}
-// MS-EXT-NEXT: int x = 3 __declspec(thread);
+// MS-EXT-NEXT: int x __declspec(thread) = 3;
 int __declspec(thread) x = 3;
 #endif //MS_EXT
 
Index: clang/test/AST/ast-print-attr.c
===
--- clang/test/AST/ast-print-attr.c
+++ clang/test/AST/ast-print-attr.c
@@ -32,3 +32,9 @@
 
 // CHECK: void fun_holds(int *a) __attribute__((ownership_holds(fun_holds, 
1)));
 void fun_holds(int *a) __attribute__((ownership_holds(fun_holds, 1)));
+
+// CHECK: int fun_var_unused() {
+// CHECK-NEXT: int x __attribute__((unused)) = 0;
+// CHECK-NEXT: return x;
+// CHECK-NEXT: }
+int fun_var_unused() { int x __attribute__((unused)) = 0; return x; }
Index: clang/lib/AST/DeclPrinter.cpp
===
--- clang/lib/AST/DeclPrinter.cpp
+++ clang/lib/AST/DeclPrinter.cpp
@@ -891,6 +891,7 @@
 D->getIdentifier())
? D->getIdentifier()->deuglifiedName()
: D->getName());
+  prettyPrintAttributes(D);
   Expr *Init = D->getInit();
   if (!Policy.SuppressInitializers && Init) {
 bool ImplicitInit = false;
@@ -919,7 +920,6 @@
 Out << ")";
 }
   }
-  prettyPrintAttributes(D);
 }
 
 void DeclPrinter::VisitParmVarDecl(ParmVarDecl *D) {


Index: clang/test/AST/ast-print-pragmas.cpp
===
--- clang/test/AST/ast-print-pragmas.cpp
+++ clang/test/AST/ast-print-pragmas.cpp
@@ -92,8 +92,9 @@
 
 #ifdef MS_EXT
 #pragma init_seg(compiler)
+/* FIXME: Trying to compile the output source results in compilation error.  */
 // MS-EXT: #pragma init_seg (.CRT$XCC){{$}}
-// MS-EXT-NEXT: int x = 3 __declspec(thread);
+// MS-EXT-NEXT: int x __declspec(thread) = 3;
 int __declspec(thread) x = 3;
 #endif //MS_EXT
 
Index: clang/test/AST/ast-print-attr.c
===
--- clang/test/AST/ast-print-attr.c
+++ clang/test/AST/ast-print-attr.c
@@ -32,3 +32,9 @@
 
 // CHECK: void fun_holds(int *a) __attribute__((ownership_holds(fun_holds, 1)));
 void fun_holds(int *a) __attribute__((ownership_holds(fun_holds, 1)));
+
+// CHECK: int fun_var_unused() {
+// CHECK-NEXT: int x __attribute__((unused)) = 0;
+// CHECK-NEXT: return x;
+// CHECK-NEXT: }
+int fun_var_unused() { int x __attribute__((unused)) = 0; return x; }
Index: clang/lib/AST/DeclPrinter.cpp
===
--- clang/lib/AST/DeclPrinter.cpp
+++ clang/lib/AST/DeclPrinter.cpp
@@ -891,6 +891,7 @@
 D->getIdentifier())
? D->getIdentifier()->deuglifiedName()
: D->getName());
+  prettyPrintAttributes(D);
   Expr *Init = D->getInit();
   if (!Policy.SuppressInitializers && Init) {
 bool ImplicitInit = false;
@@ -919,7 +920,6 @@
 Out << ")";
 }
   }
-  prettyPrintAttributes(D);
 }
 
 void DeclPrinter::VisitParmVarDecl(ParmVarDecl *D) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits