[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