[PATCH] D87331: Sema: add support for `__attribute__((__swift_error__))`

[Aaron Ballman via Phabricator via cfe-commits]

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