aaron.ballman added inline comments.
================ Comment at: clang/include/clang/Basic/Attr.td:2122 +def SwiftError : InheritableAttr { + let Spellings = [GCC<"swift_error">]; + let Args = [ ---------------- Is this attribute also supported by GCC? It looked like it wasn't from my quick check, so I'd recommend switching the spelling to be `Clang` instead. Note, this will give you `[[clang::swift_error(...)]]` in both C and C++ as well. If you only want the GNU spelling, use `GNU`. ================ Comment at: clang/include/clang/Basic/Attr.td:2128 + ]; + let Subjects = SubjectList<[Function, ObjCMethod], ErrorDiag>; + let Documentation = [SwiftErrorDocs]; ---------------- Should this apply to function-like things such as blocks or function pointers? If so, you should use `HasFunctionProto`. ================ Comment at: clang/include/clang/Basic/AttrDocs.td:3390 +Objective-C method) is imported into Swift as a throwing function, and if so, +the dynamic convention it uses. + ---------------- the dynamic -> which dynamic ================ Comment at: clang/include/clang/Basic/AttrDocs.td:3394 +parameter. Currently, the error parameter is always the last parameter of type +``NSError**`` or ``CFErrorRef*``. Swift will remove the error parameter from +the imported API. When calling the API, Swift will always pass a valid address ---------------- Canonical type or are typedefs to these types also fine? May want to mention that the type has to be cv-unqualified, but I do wonder whether something like `NSError * const *` is fine. ================ Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5536 + if (const auto *OPT = Pointee->getAs<ObjCObjectPointerType>()) + if (auto *ID = OPT->getInterfaceDecl()) + if (ID->getIdentifier() == S.getNSErrorIdent()) ---------------- `const auto *`? ================ Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5551-5552 + auto hasErrorParameter = [](Sema &S, Decl *D, const ParsedAttr &AL) -> bool { + if (D->isInvalidDecl()) + return true; + ---------------- No need to repeat this condition three times (and in some cases repeat the check). You can hoist this out of all of the lambdas and call it once. ================ Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5560 + S.Diag(AL.getLoc(), diag::err_attr_swift_error_no_error_parameter) + << AL.getNormalizedFullName() << isa<ObjCMethodDecl>(D); + return false; ---------------- You should just be able to pass in `AL` directly (the diagnostics engine handles formatting the name properly), or is there a reason you want to call `getNormalizedFullName()` explicitly? ================ Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5576 + S.Diag(AL.getLoc(), diag::err_attr_swift_error_return_type) + << AL.getNormalizedFullName() << AL.getArgAsIdent(0)->Ident->getName() + << isa<ObjCMethodDecl>(D) << /*pointer*/ 1; ---------------- Same here as above, but also, you can just pass in `Ident`. ================ Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5590 + S.Diag(AL.getLoc(), diag::err_attr_swift_error_return_type) + << AL.getNormalizedFullName() << AL.getArgAsIdent(0)->Ident->getName() + << isa<ObjCMethodDecl>(D) << /*integeral*/ 0; ---------------- Same here as above. ================ Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5596 + IdentifierLoc *Loc = AL.getArgAsIdent(0); + SwiftErrorAttr::ConventionKind convention; + if (!SwiftErrorAttr::ConvertStrToConventionKind(Loc->Ident->getName(), ---------------- `convention` -> `Convention` per usual naming rules. ================ Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5600 + S.Diag(AL.getLoc(), diag::warn_attribute_type_not_supported) + << AL.getNormalizedFullName() << Loc->Ident; + return; ---------------- Same here as above. ================ Comment at: clang/test/SemaObjC/attr-swift-error.m:10 +#endif + +typedef struct __attribute__((__objc_bridge__(NSError))) __CFError *CFErrorRef; ---------------- You should also have some tests where the attribute accepts no args, too many args, and unknown enum value. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87331/new/ https://reviews.llvm.org/D87331 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits