aaron.ballman added a comment.
It looks like several comments are left unaddressed, are you planning on making
the suggested changes?
================
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,
+ };
----------------
giulianobelinassi wrote:
> 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.
We have helper functionality for that, see `LLVM_MARK_AS_BITMASK_ENUM` from
https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/ADT/BitmaskEnum.h
================
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: ^{ }
----------------
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.
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
