From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id E18B01B301 for ; Tue, 3 Oct 2017 12:38:17 +0200 (CEST) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 03 Oct 2017 03:38:16 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,474,1500966000"; d="scan'208";a="1178101372" Received: from bricha3-mobl3.ger.corp.intel.com ([10.237.221.37]) by orsmga001.jf.intel.com with SMTP; 03 Oct 2017 03:38:14 -0700 Received: by (sSMTP sendmail emulation); Tue, 03 Oct 2017 11:38:14 +0100 Date: Tue, 3 Oct 2017 11:38:13 +0100 From: Bruce Richardson To: Adrien Mazarguil Cc: Stephen Hemminger , dev@dpdk.org Message-ID: <20171003103813.GA20140@bricha3-MOBL3.ger.corp.intel.com> References: <20170929153749.9806-1-stephen@networkplumber.org> <20171002115317.GJ3871@6wind.com> <20171002134624.GA10500@bricha3-MOBL3.ger.corp.intel.com> <20171002162106.GQ3871@6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171002162106.GQ3871@6wind.com> Organization: Intel Research and Development Ireland Ltd. User-Agent: Mutt/1.8.3 (2017-05-23) 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:38:19 -0000 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. /Bruce