Page MenuHomePhabricator

[clang] add a `swift_async_name` attribute
ClosedPublic

Authored by arphaman on Nov 30 2020, 4:17 PM.

Details

Summary

The swift_async_name attribute provides a name for a function/method that can be used to call the async overload of this method from Swift. This name specified in this attribute assumes that the last parameter in the function/method its applied to is removed when Swift invokes it, as the the Swift's await/async transformation implicitly constructs the callback.

Diff Detail

Event Timeline

arphaman created this revision.Nov 30 2020, 4:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 30 2020, 4:17 PM
arphaman requested review of this revision.Nov 30 2020, 4:17 PM
erik.pilkington accepted this revision.Dec 2 2020, 8:35 AM

LGTM!

clang/include/clang/Basic/AttrDocs.td
3640

function or method

This revision is now accepted and ready to land.Dec 2 2020, 8:35 AM
aaron.ballman requested changes to this revision.Dec 2 2020, 11:46 AM

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

clang/lib/Sema/SemaDeclAttr.cpp
5950

Do you actually need the ?: operator here, or is the conversion from bool sufficient?

6019

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.

clang/test/SemaObjC/attr-swift_name.m
184

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. :-)

197

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).

This revision now requires changes to proceed.Dec 2 2020, 11:46 AM
arphaman updated this revision to Diff 309582.Dec 4 2020, 11:01 AM
arphaman marked 3 inline comments as done.

address review comments

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.

clang/lib/Sema/SemaDeclAttr.cpp
5950

Thanks, I dropped it.

6019

Makes sense, thanks. I added a new function which should hopefully make it easier to automate in the future.

clang/test/SemaObjC/attr-swift_name.m
184

Thanks for the suggestion, I tweaked the warning text .

197

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.

doug.gregor accepted this revision.Dec 4 2020, 12:47 PM
doug.gregor added a subscriber: doug.gregor.

Thank you!

This revision is now accepted and ready to land.Dec 4 2020, 12:53 PM
This revision was automatically updated to reflect the committed changes.