[PATCH] D77683: [Docs] Make code review policy clearer about requested pre-commit reviews

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert marked an inline comment as done.
jdoerfert 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.
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77683