From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f44.google.com (mail-wm0-f44.google.com [74.125.82.44]) by dpdk.org (Postfix) with ESMTP id 8F6281B2AB for ; Tue, 3 Oct 2017 12:56:24 +0200 (CEST) Received: by mail-wm0-f44.google.com with SMTP id i124so15969097wmf.3 for ; Tue, 03 Oct 2017 03:56:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=xGUSiFVG9wcN7z+7juebGgmEOBSIuP9dhYO+Ut6b6Ik=; b=dbcPbPNws4cvDi5DlkQDpU1MWTF6BOMv/HNvwvrfIUWp4Aczd2Mj5Ppm8c9F508kek nHyMiIpOLImu0iYCM3bhK9bg0IEtAtp61aFPdmxYGJkUd49O42gGNI4M7QrHp0+4ybJv 2xSDzfzhR7R64x9J4N7f599qGuqPg/p3Oiauvojxfop9VFib9qDEEOzzuUpskgfc9fY6 wPWevJSMKGIkJEaPIETYOXFRfnzvGUXMkO0nqXRDpBI3L3wQ/BlFrQD9PHpun5UODJ0R dQBFX94z53uYIHoChEkYDI7znHtpLhQ6ao6NuErmXVmeksG/SJzEbbq9L+D+tL503tdt fCsQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=xGUSiFVG9wcN7z+7juebGgmEOBSIuP9dhYO+Ut6b6Ik=; b=m+rj5/86DFs+IVTUKMTx11j/1BFpYcJHdcKkuAtfwKg7IUubu97Vi6oNf7yNChr2Ka t+xAR8NyTuQhVWDht3uocgNI/jNswYJHrbmIHaoHTWBMBZ33EPYAc8MBXxWJ/js6W1xO /jUwg3d7NHXfIk95DOU2gvjV9fFHD4zWhcmIYu2FnOIdBcD0d0sDa5ZpEfhL3pgFCtlc 1YGDVTHmHWqgGJM8TmrfBMCP0wBnVeQQEVMgIhqOgao7nuDhxOLSN0UA4qbp0kHHX/ti wU4BcMCHVmM44D3LyMvJ0N3iicrwJHexqvzzG+TAgHGWwYMkXy0WntSC3v74ZpDtpMRU VSUA== X-Gm-Message-State: AMCzsaWlRufvoP0R7cAmx5/OQUV2sy5ZS7X8aRfkfen1RCDIkf4KEjc8 aRR9CmlvLkz1ueQvzfh66VfMZQ== X-Google-Smtp-Source: AOwi7QDxsZsIEt87L7LjidUyljuqNn5Zd2fNG00ZiNxdpQ9ibJQoVyiJQ1PIMt150lYKmQWV1iuI0Q== X-Received: by 10.28.228.213 with SMTP id b204mr12666031wmh.157.1507028184000; Tue, 03 Oct 2017 03:56:24 -0700 (PDT) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id e134sm11746920wma.31.2017.10.03.03.56.22 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 03 Oct 2017 03:56:22 -0700 (PDT) Date: Tue, 3 Oct 2017 12:56:12 +0200 From: Adrien Mazarguil To: Bruce Richardson Cc: Stephen Hemminger , dev@dpdk.org Message-ID: <20171003105612.GS3871@6wind.com> References: <20170929153749.9806-1-stephen@networkplumber.org> <20171002115317.GJ3871@6wind.com> <20171002134624.GA10500@bricha3-MOBL3.ger.corp.intel.com> <20171002162106.GQ3871@6wind.com> <20171003103813.GA20140@bricha3-MOBL3.ger.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171003103813.GA20140@bricha3-MOBL3.ger.corp.intel.com> Subject: Re: [dpdk-dev] [PATCH] checkpatch: re-enable warnings about split long strings X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 03 Oct 2017 10:56:24 -0000 On Tue, Oct 03, 2017 at 11:38:13AM +0100, Bruce Richardson wrote: > On Mon, Oct 02, 2017 at 06:21:06PM +0200, Adrien Mazarguil wrote: > > On Mon, Oct 02, 2017 at 02:46:24PM +0100, Bruce Richardson wrote: > > > On Mon, Oct 02, 2017 at 01:53:17PM +0200, Adrien Mazarguil wrote: > > > > Hi Stephen, > > > > > > > > On Fri, Sep 29, 2017 at 08:37:49AM -0700, Stephen Hemminger wrote: > > > > > The Linux kernel style policy about strings is that strings should > > > > > be always put on one line. This makes sense since a typical use case > > > > > is for a user to type the error message into a search engine or > > > > > grep, and it won't be found if split across lines. This patch just > > > > > re-enables that check. > > > > > > > > > > Yes, lots of DPDK code now splits strings, that doesn't make it > > > > > right. > > > > > > > > > > Signed-off-by: Stephen Hemminger --- > > > > > devtools/checkpatches.sh | 1 - 1 file changed, 1 deletion(-) > > > > > > > > > > diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh > > > > > index a56c41a301c0..3e6081dd673e 100755 --- > > > > > a/devtools/checkpatches.sh +++ b/devtools/checkpatches.sh @@ -44,7 > > > > > +44,6 @@ options="$options --show-types" options="$options > > > > > --ignore=LINUX_VERSION_CODE,FILE_PATH_CHANGES,\ > > > > > VOLATILE,PREFER_PACKED,PREFER_ALIGNED,PREFER_PRINTF,\ > > > > > PREFER_KERNEL_TYPES,BIT_MACRO,CONST_STRUCT,\ > > > > > -SPLIT_STRING,LONG_LINE_STRING,\ > > > > > LINE_SPACING,PARENTHESIS_ALIGNMENT,NETWORKING_BLOCK_COMMENT_STYLE,\ > > > > > NEW_TYPEDEFS,COMPARISON_TO_NULL" > > > > > > > > I'm not sure, given that the main reason for splitting strings in the > > > > first place is to avoid LONG_LINE_STRING warnings, I think we must > > > > choose between the two options. If split strings are not allowed, then > > > > long lines must be. > > > > > > > > Since checkpatches.sh is used by various automated scripts to complain > > > > loudly about problems in submissions, the above change prevents > > > > maintainers from writing long string at all (can't split and can't go > > > > past 80 columns). > > > > > > > > As a result, they will be tempted to cripple their code with nasty > > > > workarounds to shut up checkpatches.sh, we don't want that to happen. > > > > > > > > Also I think the reasons stated by original commit cf75514c8e2e are > > > > still relevant. My vote would be to keep things as is. > > > > > > > In my experience, checkpatch is smart enough to recognise when a long > > > line overflows the 80 character limit because of a single long string, > > > so the two options are not mutually exclusive. In other words, long > > > lines are not allowed except in the case where shortening the line > > > involves splitting a string. There may be a small amount of work in > > > getting checkpatch happy, i.e. by putting the string on a line on it's > > > own, but we can indeed have our cake and eat it too in this case. > > > > I can't seem to get around warnings without ignoring either SPLIT_STRING or > > LONG_LINE_STRING as of Linux v4.14-rc3's checkpatch.pl. I think you can only > > get around them by fooling it somehow. You really need to ignore at least > > LONG_LINE_STRING to meet the requirements of the commit log. > > > > However SPLIT_STRING still looks necessary to address part of cf75514c8e2e > > ("devtools: ignore warning on long log string"): > > > > "...lines that make use of PRIx64 with string concatenation will still be > > flagged if the beginning of the last string fragment begins after the 80 > > character threshold." > > > > It's not all that uncommon in my opinion. > > > If you have PRIx64 in it, it's not part of a literal string you would > grep, so it's reasonable to split there. The user cannot know what the > specific %x formatting character used is. I agree, however in that case checkpatch would complain because our configuration doesn't specify to ignore SPLIT_STRING since there is no comma separator when concatenating them. My point is that the occasional exception is still necessary for split strings, that ignoring LONG_LINE_STRING must remain either way and unnecessary warnings cause more harm than good (they need to be worked around if we enforce this rule). In short, long/split strings acceptability assessment should be left to reviewers, as it cannot be automated in all cases through checkpatch.pl. -- Adrien Mazarguil 6WIND