AaronBallman wrote: > Hi @skc7, please refrain from merging such changes without reviews from the > relevant code owners. > > cc: @AaronBallman.
Thank you for the ping and for catching this! This PR as well as a few others were landed without proper review IMO. This PR should have had a chance for clang-repl maintainers to review before landing. The same also happened here where there should have been clang-repl and codegen maintainer review, and there was fallout from landing the changes: https://github.com/llvm/llvm-project/pull/218337 (needed a revert) https://github.com/llvm/llvm-project/pull/226930 (was the reland from above, but needed another revert) In all three cases, coworkers seem to be accepting code from codeworkers (at least, I believe @skc7 @AdityaSinha149 and @yxsamliu all work for the same company), which is perfectly reasonable and happens all the time. But we should be very careful to also get reviews from the maintainers in related areas before landing, which is especially important given that each of these PRs made use of AI tools. Our [code review policies](https://llvm.org/docs/CodeReview.html#lgtm-how-a-patch-is-accepted) should probably be updated to help make this more clear, but in the meantime, please exercise more caution around landing changes. From the policy: > Our goal is to ensure community consensus around design decisions and > significant implementation choices, and one responsibility of a reviewer, > when providing an overall approval for a patch, is to be reasonably sure that > such consensus exists. If you’re not familiar enough with the community to > know, then you shouldn’t be providing final approval to commit. A reviewer > providing final approval should have commit access to the LLVM project. is intended to encourage people to accept reviews they think are in good shape, but to be cautious about landing those changes until it's clear there's consensus. > I'd suggest to revert this change and have a proper review process before > this lands. I think that's reasonable. As for the other two PRs mentioned above, I believe we're in a good state because those were already reverted. https://github.com/llvm/llvm-project/pull/217582 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
