[llvm] [llvm/llvm-project][Coroutines] ABI Object (PR #106306)

Tyler Nowicki via llvm-commits llvm-commits at lists.llvm.org
Wed Sep 4 07:51:54 PDT 2024


TylerNowicki wrote:

> > > > This patch looks good. And the proposal looks good generally (it is long and maybe I missed some details).
> > > > But the problem is.. it looks like the contents of the PR is less related to the message body. It may be confusing. And also, I think such proposal should be sent to https://discourse.llvm.org/ to get a wider visibility.
> > > 
> > > 
> > > Sorry I may not have followed the BKM here for presenting the changes. If you look at the 'Proposed PRs' part of the proposal it has a list of PRs. I split it up into separate PRs because one giant PR would not be easy to review. The first PR is from my branch (A) in my forked repo to llvm's main. The second PR is from another branch in my forked repo (B) to branch (A). And so on... So once branch (A) is merged to llvm's main then I will update the PR for (B) to point to llvm's main and that can be merged. This should allow each PR's changes to be reviewed right now without merging any PRs. Can you recommend another method to present the changes (changes that build on each other one to the next) so they can be easily reviewed?
> > 
> > 
> > No, this is not my points. I feel good with your method to split the PRs. And I feel good with the codes of the current PR and the proposal itself. But the problem is, I don't think such proposal should be presented here in a PR. But it should be sent to https://discourse.llvm.org/ to get more visibility. And for the message body of the current PR, it would be better to describe the changes actually.
> > Currently the problem is, the message body of the PR is less related to the changes in the PR.
> 
> Thanks I will create an RFC there and update this PR message.

Done. See RFC here: https://discourse.llvm.org/t/rfc-abi-objects-for-coroutines/81057

https://github.com/llvm/llvm-project/pull/106306


More information about the llvm-commits mailing list