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@+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@+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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
