[PATCH] D92355: [clang] add a `swift_async_name` attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman requested changes to this revision.
aaron.ballman added a comment.
This revision now requires changes to proceed.

There's no information in either the attribute definition in Attr.td or in the documentation as to what subject this attribute applies to.



================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5950
+        Diag(Loc, diag::warn_attr_swift_name_decl_missing_params)
+            << AL << (isa<ObjCMethodDecl>(D) ? 1 : 0);
+        return false;
----------------
Do you actually need the ?: operator here, or is the conversion from bool sufficient?


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6019
+static void handleSwiftName(Sema &S, Decl *D, const ParsedAttr &AL,
+                            bool IsAsync = false) {
   StringRef Name;
----------------
Please do not add an additional parameter to the handle functions -- we have a stretch goal of automating more of the switch behavior in SemaDeclAttr.cpp someday and we can only do that if the handle* functions have the same signatures.


================
Comment at: clang/test/SemaObjC/attr-swift_name.m:184
+
+// expected-warning at +1 {{too many parameters in '__swift_async_name__' attribute (expected 1; got 2)}}
+- (void)doSomethingY:(int)x withCallback:(CallbackTy)callback SWIFT_ASYNC_NAME("doSomething(x:y:)");
----------------
This diagnostic is too confusing for me -- a lot of people get parameter/argument confused and so this reads a bit like the *attribute* has too many arguments to it when the real issue is that the signature specified by the attribute argument has too many parameters.

How about: `too many parameters in the signature specified by the 'swift_async_name' attribute` or something along those lines?

Also, missing tests for the number of arguments passed to the attribute. :-)


================
Comment at: clang/test/SemaObjC/attr-swift_name.m:197
+void asyncNoParams(void) SWIFT_ASYNC_NAME("asyncNoParams()");
+
+// expected-warning at +1 {{'__swift_async_name__' attribute cannot be applied to this declaration}}
----------------
It's not clear whether we should have additional tests around other subjects - like, can I apply this attribute to a virtual class function in C++? If you can apply it to a C++ class method, does the implicit `this` parameter count when checking for the right parameter count?

Should also have a test that the attribute applies to a vanilla function (the only test currently fails because the function has no args, but we should show an explicitly accepted case as well).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92355/new/

https://reviews.llvm.org/D92355