[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