[PATCH] D96175: [clang] Add support for attribute 'swift_async_error'

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM modulo some testing requests. Thanks!



================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5977
+    checkSwiftAsyncErrorBlock(S, D, ErrorAttr, AsyncAttr);
+}
+
----------------
erik.pilkington wrote:
> aaron.ballman wrote:
> > Should there be a diagnostic to combine `swift_error` and `swift_async_error` on the same function? Similarly, should there be a diagnostic when adding `swift_async_error` to a function that isn't (eventually) marked `swift_async`?
> My understanding is that `swift_error` and `swift_async_error` don't conflict with each other. `swift_error` describes how the non-async swift function handles errors, whereas `swift_async_error` describes how the swift async function handles errors. Since both swift functions are generated, I think it can make sense to have both attributes on one ObjC function. 
> 
> I think it can make sense to have `swift_async_error` without `swift_async`, since the swift importer infers some functions are async without the `swift_async` attribute using heuristics based on parameter names. I guess we could mimic those heuristics here to diagnose when `swift_async_error` doesn't make sense alone, but ISTM that it makes more sense to do that check in the swift importer.
Thanks for the information!

> I think it can make sense to have both attributes on one ObjC function.

Fine by me then! Can you add a test case with a comment showing that this is explicitly expected to be okay?

> I think it can make sense to have swift_async_error without swift_async

Also fine by me then! I don't think we need to go to great effort here to diagnose that (the swift importer can gain extra logic if that's useful). I think this is also worth a test case with a comment.

(In both cases, I'm thinking mostly about code archeology projects in the future to answer "did they think of" kind of questions.)


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

https://reviews.llvm.org/D96175