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