On Tue, Apr 19, 2016 at 07:21:20PM +0200, Mason wrote:
> 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 <marc_gonza...@sigmadesigns.com>
> >>>>
> >>>> Commit 0881841f7e78 changed "if (ret != 0)" to "if (!ret)"
> >>>>
> >>>> Fixes: 0881841f7e78 ("Replace code by clocksource_mmio_init")
> >>>> Signed-off-by: Marc Gonzalez <marc_gonza...@sigmadesigns.com>
> >>>> ---
> >>>
> >>> 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
yes.
> 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
yes.
> Do you agree that 2) would be a (minor) improvement?
> If not, I will not bother with the patch.
Usually people are using 'ret'
grep -r "ret =" drivers/ | wc -l
76754
or are using 'err'
grep -r "err =" drivers/ | wc -l
27940
Up to you ...
Thanks.
-- Daniel
