Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754581AbcDSRVk (ORCPT ); Tue, 19 Apr 2016 13:21:40 -0400 Received: from smtp4-g21.free.fr ([212.27.42.4]:5647 "EHLO smtp4-g21.free.fr" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754087AbcDSRVj (ORCPT ); Tue, 19 Apr 2016 13:21:39 -0400 Subject: Re: [PATCH] clocksource/drivers/tango-xtal: Fix incorrect test To: Daniel Lezcano Cc: Thomas Gleixner , Sebastian Frias , Mans Rullgard , LKML References: <57162153.4070707@free.fr> <20160419131330.GA419@linaro.org> <57163B1F.7050402@free.fr> <20160419145946.GB419@linaro.org> From: Mason Message-ID: <57166910.2080703@free.fr> Date: Tue, 19 Apr 2016 19:21:20 +0200 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:42.0) Gecko/20100101 Firefox/42.0 SeaMonkey/2.39 MIME-Version: 1.0 In-Reply-To: <20160419145946.GB419@linaro.org> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2081 Lines: 59 On 19/04/2016 16:59, Daniel Lezcano wrote: > On Tue, Apr 19, 2016 at 04:05:19PM +0200, Mason wrote: >> On 19/04/2016 15:13, Daniel Lezcano wrote: >> >>> On Tue, Apr 19, 2016 at 02:15:15PM +0200, Mason wrote: >>> >>>> From: Marc Gonzalez >>>> >>>> Commit 0881841f7e78 changed "if (ret != 0)" to "if (!ret)" >>>> >>>> Fixes: 0881841f7e78 ("Replace code by clocksource_mmio_init") >>>> Signed-off-by: Marc Gonzalez >>>> --- >>> >>> Please resend the patch with the fix only, without s/ret/err/ >> >> As I wrote on IRC, I think it is misguided to consider variable >> renaming as not part of the fix. A properly named variable helps >> reviewers by communicating intent. >> >> Had I named the variable 'err' in the first place, would you have >> introduced the bug by writing >> >> if (!err) { >> pr_err("registration failed"); >> } >> >> or would if (!err) have jumped out for an error path? >> (Not a rhetorical question; if you say it would not have helped, >> then I guess my mental workflow is different.) > > Ok I won't argue for a stupid variable name. > > The point is we are at v4.6-rc4 and even if the change is obvious, it is a > good practice to do a simple change: > > - if (!ret) { > + if (ret) { > > Why ? Because maintainers have a lot of code to review, and removing the > noise as much as possible helps them to make their life easier especially > when they have to pay double attention for fixes at RC. > > If the 'ret' name is a problem for you, just send another patch for v4.7 to > change the name. I want to be sure I understand, please correct me if I'm wrong. 1) you have already committed the minimal fix above (changing only the test, and keeping the original variable name) and this will be pushed to linux-next for the upcoming v4.6-rc 2) if I want to change the variable name, I can send another patch, to be pushed in the next merge window, for v4.7 Do you agree that 2) would be a (minor) improvement? If not, I will not bother with the patch. Regards.