hubert.reinterpretcast added inline comments.
================ Comment at: llvm/docs/CodeReview.rst:34 +uncertainty, a patch should be reviewed prior to being committed. If pre-commit +code reviewes in a particular area have been requested, code should clear a +significantly higher bar, e.g., fixes, to be commited without review. ---------------- arsenm wrote: > jdoerfert wrote: > > hubert.reinterpretcast wrote: > > > If I understand this correctly, this is meant to cover situations where > > > reviewers are active in an area and indicated an interest in reviewing > > > basically everything. This seems redundant to likely-community-consensus. > > > If someone is active in an area of the code, they would already know > > > which reviewers are active and the area-specific culture. If someone is > > > not active in an area of the code, the likely-community-consensus > > > requirement should be enough to cause them to request a review. > > > > > > Perhaps a clarification that "the likely-community-consensus requirement > > > may vary depending on the specific area of the code" is enough. Or > > > perhaps, "likely-community-consensus is not restricted to the content of > > > the patch, but also includes the review process and culture". > > > If I understand this correctly, this is meant to cover situations where > > > reviewers are active in an area and indicated an interest in reviewing > > > basically everything. > > > > Pretty much, yes. > > > > > This seems redundant to likely-community-consensus. If someone is active > > > in an area of the code, they would already know which reviewers are > > > active and the area-specific culture. If someone is not active in an area > > > of the code, the likely-community-consensus requirement should be enough > > > to cause them to request a review. > > > > I would have agreed with you but this is in practice unfortunately not true. > > > > > Perhaps a clarification that "the likely-community-consensus requirement > > > may vary depending on the specific area of the code" is enough. Or > > > perhaps, "likely-community-consensus is not restricted to the content of > > > the patch, but also includes the review process and culture". > > > > I'm fine with either. At the end of the day this is supposed to make it > > clear that reviews cannot be circumvented while pointing to this rule if it > > is clear reviews were requested. > Typo reviewes, and commited I'd suggest a process of checking for Herald rules if I knew of an easy way to know how Herald would react for a commit without posting a review. I don't know of such a way though. Suggestion for wording: The likely-community-consensus is not restricted to the content of the patch but also includes the review process and culture, which can vary by context. A strong pre-commit culture in an area of the code should be respected. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77683/new/ https://reviews.llvm.org/D77683 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
