[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 &out,
+                               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 &out,
+                                        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