[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