aaron.ballman added inline comments.
================ 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: > I think we should use an `enum class` just so we don't steal these identifiers at the global scope within this file, WDYT? ================ Comment at: clang/lib/AST/DeclPrinter.cpp:264-266 + // 1- should be print on the left side of a decl. + // 2- should be print on the middle of a decl right after the type. + // 3- should be print on the right side of a decl. ---------------- ================ Comment at: clang/lib/AST/DeclPrinter.cpp:267-268 + // 3- should be print on the right side of a decl. + AttrPrintLoc attrloc = Right; // We default to middle. + attr::Kind kind = A->getKind(); + ---------------- Minor style nits. ================ 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 ---------------- This should also handle C2x attributes, so I'd use `isStandardAttributeSyntax()` instead. ================ 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 ---------------- There are other attributes for which this is true as well, like `enable_if`, thread safety annotations, etc. ================ Comment at: clang/lib/AST/DeclPrinter.cpp:286-287 + attrloc = Left; + } else if (D->getAsFunction() && + D->getAsFunction()->isThisDeclarationADefinition()) { + // In case Decl is a function with a body, then attrs should be print ---------------- ================ Comment at: clang/lib/AST/DeclPrinter.cpp:664-665 + + std::string Proto; + llvm::raw_string_ostream OS(Proto); + ---------------- What's the purpose to hoisting these two lines? ================ 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: ^{ } ---------------- I can't quite tell if this change is good, bad, or just different. 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