From patchwork Wed Nov 21 09:42:01 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eilon Greenstein X-Patchwork-Id: 200616 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 3A4E92C0091 for ; Wed, 21 Nov 2012 20:43:11 +1100 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754133Ab2KUJmw (ORCPT ); Wed, 21 Nov 2012 04:42:52 -0500 Received: from mms2.broadcom.com ([216.31.210.18]:3060 "EHLO mms2.broadcom.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752729Ab2KUJms (ORCPT ); Wed, 21 Nov 2012 04:42:48 -0500 Received: from [10.9.200.133] by mms2.broadcom.com with ESMTP (Broadcom SMTP Relay (Email Firewall v6.5)); Wed, 21 Nov 2012 01:40:01 -0800 X-Server-Uuid: 4500596E-606A-40F9-852D-14843D8201B2 Received: from mail-irva-13.broadcom.com (10.11.16.103) by IRVEXCHHUB02.corp.ad.broadcom.com (10.9.200.133) with Microsoft SMTP Server id 8.2.247.2; Wed, 21 Nov 2012 01:42:14 -0800 Received: from [10.185.6.73] (lb-tlvb-eilong.il.broadcom.com [10.185.6.73]) by mail-irva-13.broadcom.com (Postfix) with ESMTP id 4B17E40FEB; Wed, 21 Nov 2012 01:42:36 -0800 (PST) Message-ID: <1353490921.6559.40.camel@lb-tlvb-eilong.il.broadcom.com> Subject: Re: [PATCH v2] checkpatch: add double empty line check From: "Eilon Greenstein" Reply-to: eilong@broadcom.com To: "Joe Perches" , "Andy Whitcroft" cc: "David Rientjes" , linux-kernel@vger.kernel.org, netdev Date: Wed, 21 Nov 2012 11:42:01 +0200 In-Reply-To: <1353454874.17819.41.camel@joe-AO722> References: <1353151057.14327.18.camel@lb-tlvb-eilong.il.broadcom.com> <20121120115239.GA7955@dm> <1353448728.17819.33.camel@joe-AO722> <20121120231930.GA27492@localhost> <1353454874.17819.41.camel@joe-AO722> Organization: Broadcom X-Mailer: Evolution 3.2.3-0ubuntu6 MIME-Version: 1.0 X-WSS-ID: 7CB27EFB3R06543203-01-01 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Tue, 2012-11-20 at 15:41 -0800, Joe Perches wrote: > On Tue, 2012-11-20 at 23:19 +0000, Andy Whitcroft wrote: > > On Tue, Nov 20, 2012 at 01:58:48PM -0800, Joe Perches wrote: > > > > > +# check for multiple blank lines, warn only on the second one in a block > > > + if ($rawline =~ /^.\s*$/ && > > > + $prevrawline =~ /^.\s*$/ && > > > + $linenr != $last_blank_linenr + 1) { > > > + CHK("DOUBLE_EMPTY_LINE", > > > + "One blank line separating blocks is generally sufficient\n" . $herecurr); > > > + $last_blank_linenr = $linenr; > > > + } > > > + > > > # check for line continuations in quoted strings with odd counts of " > > > if ($rawline =~ /\\$/ && $rawline =~ tr/"/"/ % 2) { > > > WARN("LINE_CONTINUATIONS", > > > > Pretty sure that will fail with combination which have removed lines. > > Not as far as I can tell. > Deleted lines followed by inserted lines seem > to work OK. > > This check is located after the test that ensures > the current $line/$rawline is an insertion. > But you do not look at the next line, so you will miss something like that: > > I have a version here which I am testing with the combinations I have > > isolated to far ... > > Enjoy. > Can you please test my proposal against those combinations too? > The way I see it, we have to handle the following cases: a. The patch adds more than a single consecutive empty line - easy enough, the only "problem" here is to warn only once and there are many ways to do that. b. The patch is adding a new empty line after an existing empty line - for that, we must check the previous line. c. The patch is adding a new empty line before an existing empty line - for that, we must check the next line. If we are already checking the next line, we can tell if this is the last empty line added and therefore do not need to save anything in order to warn only once per block. My version of the patch addresses all 3 cases above, and I do not see how we can do it without looking at the next line and the previous line - so I think it is a valid approach. The only identified down side is that it might fail to warn about double empty lines if we will find a diff utility that will add the deleted lines after the inserted lines - but even in that case, it will not generate any annoying false positives and no other perl warnings. To address this issue, I can add a loop that will look forward if the next line after a newly added empty line is a deleted line, but I think this is excessive and I will only be able to test it on manually generated files since the diff utilities I'm familiar with are behaving nicely and delete before adding. Anyway, I'm looking forward for your version. Thanks, Eilon --- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/test.c b/test.c index e3c46d4..e1c6ffc 100644 --- a/test.c +++ b/test.c @@ -15,7 +15,8 @@ * something * something * something - * next line was already empty */ + * next line was already empty, but I'm adding another one now*/ + /* something else * something else