From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ingo Molnar Date: Sun, 15 Feb 2009 20:08:50 +0000 Subject: Re: [PATCH] Remove errors caught by checkpatch.pl in Message-Id: <20090215200850.GA28011@elte.hu> List-Id: References: In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Manish Katiyar Cc: LKML , kernel-janitors@vger.kernel.org * Manish Katiyar wrote: > Hi, > > Below patch removes some errors generated by checkpatch.pl in > kernel/signal.c. Caught by Ingo's code-quality script. > > > Signed-off-by: Manish Katiyar > --- > kernel/signal.c | 47 +++++++++++++++++++++++------------------------ > 1 files changed, 23 insertions(+), 24 deletions(-) Note, your patch has line-wrap problems, such as: > +++ b/kernel/signal.c > @@ -89,20 +89,20 @@ static inline int has_pending_signals(sigset_t > *signal, sigset_t *blocked) > switch (_NSIG_WORDS) { Causing: patch: **** malformed patch at line 23: *signal, sigset_t *blocked) See Documentation/email-clients.txt. Also, kernel/signal.c needs a thorough cleanup, and your patch only handles about a third of the errors+warnings: before: total: 61 errors, 28 warnings, 3 checks, 2615 lines checked after: total: 30 errors, 24 warnings, 3 checks, 2614 lines checked If then it's best to bring the count down very close to zero (so that only the obvious checkpatch false positives are left), and also have a really good human-coder look at signal.c's: - structure and code flow - variable naming - include files section - general splitup and function ordering - comment style consistency etc. - to turn it into a really modern, nice-to-look-at and hackable Linux kernel file. The checkpatch motivated cleanups really only tell us half of the story that is usually in such a long-forgotten file - and by fixing the checkpatch warnings only it gives us a false impression of cleanliness. So the checkpatch fixes should go hand in hand with more grounds-up cleanups. (which can all still stay at the pure style level, so that the .o output does not change - for easy verification.) Ingo From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754313AbZBOUJT (ORCPT ); Sun, 15 Feb 2009 15:09:19 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1751307AbZBOUJD (ORCPT ); Sun, 15 Feb 2009 15:09:03 -0500 Received: from mx2.mail.elte.hu ([157.181.151.9]:37619 "EHLO mx2.mail.elte.hu" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751019AbZBOUJC (ORCPT ); Sun, 15 Feb 2009 15:09:02 -0500 Date: Sun, 15 Feb 2009 21:08:50 +0100 From: Ingo Molnar To: Manish Katiyar Cc: LKML , kernel-janitors@vger.kernel.org Subject: Re: [PATCH] Remove errors caught by checkpatch.pl in kernel/signal.c Message-ID: <20090215200850.GA28011@elte.hu> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.18 (2008-05-17) X-ELTE-VirusStatus: clean X-ELTE-SpamScore: -1.5 X-ELTE-SpamLevel: X-ELTE-SpamCheck: no X-ELTE-SpamVersion: ELTE 2.0 X-ELTE-SpamCheck-Details: score=-1.5 required=5.9 tests=BAYES_00 autolearn=no SpamAssassin version=3.2.3 -1.5 BAYES_00 BODY: Bayesian spam probability is 0 to 1% [score: 0.0000] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * Manish Katiyar wrote: > Hi, > > Below patch removes some errors generated by checkpatch.pl in > kernel/signal.c. Caught by Ingo's code-quality script. > > > Signed-off-by: Manish Katiyar > --- > kernel/signal.c | 47 +++++++++++++++++++++++------------------------ > 1 files changed, 23 insertions(+), 24 deletions(-) Note, your patch has line-wrap problems, such as: > +++ b/kernel/signal.c > @@ -89,20 +89,20 @@ static inline int has_pending_signals(sigset_t > *signal, sigset_t *blocked) > switch (_NSIG_WORDS) { Causing: patch: **** malformed patch at line 23: *signal, sigset_t *blocked) See Documentation/email-clients.txt. Also, kernel/signal.c needs a thorough cleanup, and your patch only handles about a third of the errors+warnings: before: total: 61 errors, 28 warnings, 3 checks, 2615 lines checked after: total: 30 errors, 24 warnings, 3 checks, 2614 lines checked If then it's best to bring the count down very close to zero (so that only the obvious checkpatch false positives are left), and also have a really good human-coder look at signal.c's: - structure and code flow - variable naming - include files section - general splitup and function ordering - comment style consistency etc. - to turn it into a really modern, nice-to-look-at and hackable Linux kernel file. The checkpatch motivated cleanups really only tell us half of the story that is usually in such a long-forgotten file - and by fixing the checkpatch warnings only it gives us a false impression of cleanliness. So the checkpatch fixes should go hand in hand with more grounds-up cleanups. (which can all still stay at the pure style level, so that the .o output does not change - for easy verification.) Ingo