[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]

On Jun 4, 2012, at 12:46 PM, Chandler Carruth <chandlerc at google.com> wrote:
> Ok, I see it as a significant change, but maybe that's inaccurate. The things I don't see as significant are bug fixes, tuning performance, refactoring following an already accepted design direction, etc.
> 
> Certainly this is fuzzy ground, but I think it's pretty clear to each developer what constitues "significant" relative to their own historical contributions. The first time I touched the x86 backend, I sent even bug fix patches for pre-commit review because I didn't have the faintest clue what I was doing. ;] They were all significant to me.

Fair enough.  This change was based on code already in the ARM target so it definitely seemed to me like it didn't need to be reviewed ahead of time.

> 
> In this case, Manman mailed out the patch for pre-commit review, and so it seemed to qualify for 'significant' compared to his past experience (which afaict, is limited). Certainly the comments that have ensued have reinforced this to my eyes...
> 
> 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.
> 
> I didn't ask for it to be reverted. I think revert is a strong thing, and I like reserving that for when the patch is actively broken.
> 
> My frustration was that the pre-commit review was started, had not concluded, and still had constructive feedback coming. Switching (without notice) to post-commit review seems unwarranted and frustrating. That said, I was trying mostly to give feedback for subsequent reviews. If I feel like a patch need sto be reverted, I'll say so explicitly. ;]

OK, that makes sense. 

>  
>  (There was some question whether it might have been causing a regression, so reverting on that basis was a good idea.)
> 
> Exactly. Regression or breakage -> revert. Otherwise, fix-forward. I think we're on the same page there.
> 
> My comment was just that I thought in the future, pre-commit review should probably have continued a few more rounds, and was trying to help Manman become better calibrated to the review process within the community.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120604/e78a2239/attachment.html>