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

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

In D92355#2429117 <https://reviews.llvm.org/D92355#2429117>, @aaron.ballman wrote:

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

Added a subject list to the attribute.



================
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;
----------------
aaron.ballman wrote:
> Do you actually need the ?: operator here, or is the conversion from bool sufficient?
Thanks, I dropped it.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6019
+static void handleSwiftName(Sema &S, Decl *D, const ParsedAttr &AL,
+                            bool IsAsync = false) {
   StringRef Name;
----------------
aaron.ballman wrote:
> 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.
Makes sense, thanks. I added a new function which should hopefully make it easier to automate in the future.


================
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:)");
----------------
aaron.ballman wrote:
> 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. :-)
Thanks for the suggestion, I tweaked the warning text .


================
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}}
----------------
aaron.ballman wrote:
> 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).
That's a good point. Added a regular function test as well. We don't need C++ support for this right now, but it might make sense to have it in the future when Swift and C++ interoperate, so added a test for C++ method as well.


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

https://reviews.llvm.org/D92355