[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