giulianobelinassi added a comment.
Hi. See inline answers. I will send the new version in a few minutes.
================
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,
+ };
----------------
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.
================
Comment at: clang/lib/AST/DeclPrinter.cpp:272
+ // to classify each of them.
+ if (A->isCXX11Attribute()) {
+ // C++11 onwards attributes can not be placed on left side. Output on
----------------
aaron.ballman wrote:
> This should also handle C2x attributes, so I'd use
> `isStandardAttributeSyntax()` instead.
Done.
================
Comment at: clang/lib/AST/DeclPrinter.cpp:279
+ attrloc = Right;
+ } else if (kind == attr::DiagnoseIf) {
+ // DiagnoseIf should be print on the right side because it may refer to
----------------
aaron.ballman wrote:
> There are other attributes for which this is true as well, like `enable_if`,
> thread safety annotations, etc.
I have expanded this to enable_if, as it is trivial. The thread safety
annotations is not that trivial and seems to not trigger any test failure. So I
think I will leave this to a next patch that properly parses the attributes to
find references to symbols declared in function argument.
================
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:
> 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.
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
