[llvm-commits] [llvm] r157831 - in /llvm/trunk: lib/Target/X86/X86InstrArithmetic.td lib/Target/X86/X86InstrInfo.cpp lib/Target/X86/X86InstrInfo.h test/CodeGen/X86/jump_sign.ll

Bob Wilson bob.wilson at apple.com
Mon Jun 4 12:26:15 PDT 2012


On Jun 2, 2012, at 3:55 PM, Chandler Carruth <chandlerc at google.com> wrote:

> 
> On Fri, Jun 1, 2012 at 12:49 PM, Manman Ren <mren at apple.com> wrote:
> >
> > Author: mren
> > Date: Fri Jun  1 14:49:33 2012
> > New Revision: 157831
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=157831&view=rev
> > Log:
> > X86: peephole optimization to remove cmp instruction
> 
> I'm frustrated that you waited a total of 3 hours for review on the final version of this patch before committing it, w/o getting an LGTM. Eric even made comments on that thread, which he has had to repeat here. Please try to give the code review process a chance.
> 

I don't understand your frustration, Chandler.

I'm not questioning your specific concerns about the patch, but you seem to have a different understanding of the process.  Here is what the LLVM Developer Policy says about code review:
LLVM has a code review policy. Code review is one way to increase the quality of software. We generally follow these policies:

All developers are required to have significant changes reviewed before they are committed to the repository.
Code reviews are conducted by email, usually on the llvm-commits list.
Code can be reviewed either before it is committed or after. We expect major changes to be reviewed before being committed, but smaller changes (or changes where the developer owns the component) can be reviewed after commit.
The developer responsible for a code change is also responsible for making all necessary review-related changes.
Code review can be an iterative process, which continues until the patch is ready to be committed.
This peephole optimization hardly seems like a "significant change" or a "major change", so according to our policy, it can be reviewed after commit.

In this case, I don't think it was necessary for Manman to revert the patch, at least not because of the code review comments.  (There was some question whether it might have been causing a regression, so reverting on that basis was a good idea.)  As long as a patch isn't breaking anything, and the code review feedback can be addressed in an incremental way, I think reverting is generally a bad idea.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120604/7ee01f66/attachment.html>


More information about the llvm-commits mailing list