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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
