From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755385AbcH1KYq (ORCPT ); Sun, 28 Aug 2016 06:24:46 -0400 Received: from mail2-relais-roc.national.inria.fr ([192.134.164.83]:9027 "EHLO mail2-relais-roc.national.inria.fr" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755268AbcH1KYo (ORCPT ); Sun, 28 Aug 2016 06:24:44 -0400 X-IronPort-AV: E=Sophos;i="5.28,590,1464645600"; d="scan'208";a="234301856" Date: Sun, 28 Aug 2016 12:24:40 +0200 (CEST) From: Julia Lawall X-X-Sender: jll@hadrien To: SF Markus Elfring cc: Julia Lawall , linux-cris-kernel@axis.com, Adam Buchbinder , Dave Hansen , Ingo Molnar , Jesper Nilsson , Jiri Kosina , Mikael Starvik , Thomas Gleixner , LKML , kernel-janitors@vger.kernel.org, Paolo Bonzini Subject: Re: [PATCH 5/8] cris-cryptocop: Move an assignment for the variable "nooutpages" in cryptocop_ioctl_process() In-Reply-To: <02c343e7-a7ac-26d1-4e79-ae50d3ec6dbb@users.sourceforge.net> Message-ID: References: <566ABCD9.1060404@users.sourceforge.net> <2a70980b-54e3-812c-b76e-6b64640c469c@users.sourceforge.net> <02c343e7-a7ac-26d1-4e79-ae50d3ec6dbb@users.sourceforge.net> User-Agent: Alpine 2.10 (DEB 1266 2009-07-14) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, 28 Aug 2016, SF Markus Elfring wrote: > >> +++ b/arch/cris/arch-v32/drivers/cryptocop.c > >> @@ -2469,7 +2469,7 @@ static int cryptocop_ioctl_process(struct inode *inode, struct file *filp, unsig > >> struct page **inpages = NULL; > >> struct page **outpages = NULL; > >> int noinpages = 0; > >> - int nooutpages = 0; > >> + int nooutpages; > >> > >> struct cryptocop_desc descs[5]; /* Max 5 descriptors are needed, there are three transforms that > >> * can get connected/disconnected on different places in the indata. */ > >> @@ -2695,6 +2695,8 @@ static int cryptocop_ioctl_process(struct inode *inode, struct file *filp, unsig > >> err = -ENOMEM; > >> goto free_inpages; > >> } > >> + } else { > >> + nooutpages = 0; > > > > Why is it better? 4 characters have becomes 2 lines. > > I suggest to express in a more precise way where this variable is needed > actually. The variable is used in the cleanup code at the end of the function. Thus it conceptually has global scope, and it is completely reasonable to initialize it at the beginning of its function, along with noinpages. This code is horrible in so many ways: no space before {, lots of 0 initializations instead of kzalloc, random use of local cleanup code and a label at the end of the function, the use of DEBUG, the use of printk, the use of the very long function name in strings instead of __func__, constants on the left of a != test, etc. On the other hand there are also very few commits on this code, and even fewer that are specific to this code, so perhaps no one cares about it. julia > > * It would also be an update candidate for the refactoring "Reduce the scope of a variable", wouldn't it? > > * Or would the refactoring "Split the implementation of a function into further functions" more appropriate here? > > Regards, > Markus > -- > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >