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