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