[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

Wed Jul 26 04:32:53 PDT 2023

ChuanqiXu added a subscriber: rsmith.
ChuanqiXu added a comment.

There are several topics now. Let's discuss them separately to make things clear:

(1) Should we add such warning?

> Does the use-case mentioned above make sense (allow C++20, disallow coroutines for code using Clang 17)?

This looks like a coding guide to me. So I feel it may be better to follow what other coding guides does. As far as I know, it is rare to insert warnings to the compilers for a specific coding guide.

(2) Is it OK to claim coroutines as released?

The reason why we marked it is in the above link. And I think the key question here is that how stable coroutines is in production? And as far as I know, all the industry users (Facebook, Alibaba, Seastar, I am not sure if Google has used it in production) don't feel bad with the current status. Also there a lot of relatively smaller projects which have been using coroutines for a long time. So personally, I feel it is OK to mark it as completed.

Also my intention to mark it as released is primarily for smaller groups. Since they may lack the confidence to use it in serious situations. But I feel (and always tell) they can. Since we already tried it for a long time.

(3) What's the practical affects for these bugs?

https://github.com/llvm/llvm-project/issues/57638: it is about **the performance  at the O0 level**. So I feel the priority is relatively low. Since generally we don't care about the performance within O0. (I know there are some against opinions)

https://github.com/llvm/llvm-project/issues/63818: it looks like only related to statement expressions. Maybe it is a good idea to emit warnings for using coroutines in statement expressions. Maybe we can fix this in the frontend as @rsmith said.

https://github.com/llvm/llvm-project/issues/58459: while we need to look into the reasons, from the reproducer, it looks like it only appears if the return type of the `await_resume()` function is `std::nullptr_t`, which is rare. Also I feel this is a frontend issue.

https://github.com/llvm/llvm-project/issues/56301: this is the most severe issue among these reports.  This issue may only be possible with a special `await_suspend` implementation. And ecosystem of coroutines is that some people write coroutine library and other people use these coroutine libraries. Then it won't be a problem for the wide users if the library don't use a special `await_suspend` implementation. And this is why the existing users of coroutines don't meet the issue.

(4) How should we handle the issue?

I have an idea about https://github.com/llvm/llvm-project/issues/56301 and it requires us to implement a new alias analysis pass for coroutines.  And I can image there will be a lot of works and I don't think I can make it in the recent 2~3 months. (This may be the reason that I always delay it...). I can try to promote its priority but I can't promise when can I fix it...

For https://github.com/llvm/llvm-project/issues/57638, I feel it is hard to fix and the practical affects of it doesn't matter really. So I think we don't need to discuss it now.

I don't have insights about https://github.com/llvm/llvm-project/issues/63818 and https://github.com/llvm/llvm-project/issues/58459. I feel https://github.com/llvm/llvm-project/issues/58459 may be an oversight somewhere but I didn't look into it.

Hope this helps.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156247