From mboxrd@z Thu Jan 1 00:00:00 1970 From: Adrien Mazarguil Subject: Re: [PATCH] checkpatch: re-enable warnings about split long strings Date: Tue, 3 Oct 2017 12:56:12 +0200 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 Cc: Stephen Hemminger , dev@dpdk.org To: Bruce Richardson 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) Content-Disposition: inline In-Reply-To: <20171003103813.GA20140@bricha3-MOBL3.ger.corp.intel.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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