[PATCH] Include a clearer policy about what's ok/nok to speed up code reviews.

[Tom Stellard]

On Fri, Aug 23, 2013 at 01:50:59AM -0700, Manuel Klimek wrote:
>   Some rewording based on feedback.
> 
> http://llvm-reviews.chandlerc.com/D1472
> 
> CHANGE SINCE LAST DIFF
>   http://llvm-reviews.chandlerc.com/D1472?vs=3666&id=3695#toc
> 
> Files:
>   docs/DeveloperPolicy.rst
> 
> Index: docs/DeveloperPolicy.rst
> ===================================================================
> --- docs/DeveloperPolicy.rst
> +++ docs/DeveloperPolicy.rst
> @@ -128,7 +128,24 @@
>     all necessary review-related changes.
>  
>  #. Code review can be an iterative process, which continues until the patch is
> -   ready to be committed.
> +   ready to be committed. Specifically, once a patch is sent out for review, it
> +   needs an explicit "looks good" before it is submitted. Do not assume silent
> +   approval, or request active objections to the patch with a deadline.
> +
> +Sometimes code reviews will take longer than you would hope for, especially for
> +larger features. Accepted ways to speed up review times for your patches are:
> +
> +* Review other people's patches. If you help out, everybody will be more
> +  willing to do the same for you; goodwill is our currency.

I think this is a good suggestion, but I also think it would be more
effective if we were able to expand the pool of people who are able to
approve patches for commit.

As it stands now, there are two classes of developers: The "approvers" and
then "everyone else".  I think sometimes developers in the "everyone else"
category may feel like their review is not really worth much, since even
after they give a LGTM the patch still can't be committed.

And maybe part of the problem is that some of the "everyone else"
developers are actually "approvers" they just don't know it or don't
feel comfortable approving patches unless Chris or someone else says
they can.

The other problem I see is that the "approvers" can usually just
commit their own patches without having to go through the pre-commit review
process.  So, it is difficult to build goodwill with them since there is
usually very little opportunity to review their patches.  I know there
is always the opportunity for post-commit review, but the goodwill comes
from taking the time to review somebody's patch and helping them get it
into TOT quickly, as opposed to them having to wait a week or two to
commit.

In conclusion, while I think this is a good suggestion, we should also
clarify in the developers policy who can and cannot approve patches for
commit, because I think if the pool of "approvers" is well defined (and
maybe a little bigger) it will give more of an incentive to developers
in the "everyone else" category to review other patches and help reduce
the time developers need to wait for review.

Thanks,
Tom

> +* Ping the patch. If it is urgent, provide reasons why it is important to you to
> +  get this patch landed and ping it every couple of days. If it is
> +  not urgent, the common courtesy ping rate is one week. Remember that you're
> +  asking for valuable time from other professional developers.
> +* Ask for help on IRC. Developers on IRC will be able to either help you
> +  directly, or tell you who might be a good reviewer.
> +* Split your patch into multiple smaller patches that build on each other. The
> +  smaller your patch, the higher the probability that somebody will take a quick
> +  look at it.
>  
>  Developers should participate in code reviews as both reviewers and
>  reviewees. If someone is kind enough to review your code, you should return the

> Index: docs/DeveloperPolicy.rst
> ===================================================================
> --- docs/DeveloperPolicy.rst
> +++ docs/DeveloperPolicy.rst
> @@ -128,7 +128,24 @@
>     all necessary review-related changes.
>  
>  #. Code review can be an iterative process, which continues until the patch is
> -   ready to be committed.
> +   ready to be committed. Specifically, once a patch is sent out for review, it
> +   needs an explicit "looks good" before it is submitted. Do not assume silent
> +   approval, or request active objections to the patch with a deadline.
> +
> +Sometimes code reviews will take longer than you would hope for, especially for
> +larger features. Accepted ways to speed up review times for your patches are:
> +
> +* Review other people's patches. If you help out, everybody will be more
> +  willing to do the same for you; goodwill is our currency.
> +* Ping the patch. If it is urgent, provide reasons why it is important to you to
> +  get this patch landed and ping it every couple of days. If it is
> +  not urgent, the common courtesy ping rate is one week. Remember that you're
> +  asking for valuable time from other professional developers.
> +* Ask for help on IRC. Developers on IRC will be able to either help you
> +  directly, or tell you who might be a good reviewer.
> +* Split your patch into multiple smaller patches that build on each other. The
> +  smaller your patch, the higher the probability that somebody will take a quick
> +  look at it.
>  
>  Developers should participate in code reviews as both reviewers and
>  reviewees. If someone is kind enough to review your code, you should return the

> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits