From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id B6BB446140; Tue, 28 Jan 2025 16:56:03 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 44D0B40150; Tue, 28 Jan 2025 16:56:03 +0100 (CET) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id A930540144 for ; Tue, 28 Jan 2025 16:56:00 +0100 (CET) Received: by linux.microsoft.com (Postfix, from userid 1213) id ACF282037160; Tue, 28 Jan 2025 07:55:59 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com ACF282037160 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1738079759; bh=jLx7TdiPqvEqPuL5rgYnf7aY+QGrMPiMRY0BEkW0a40=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=Ybu3xIMI5AXq6jJ1wqPpwSA+ca36X4KTtfvNg7wThnBvDijMoPA3zncF/Gmc0l3WB XZH2OHboQQShCq7Y8Xa/G+gj+vES/AtOf29Ks+xvL/kqO6nU9Ud+1IYFdzjbw61tHa HENvsZvt1eI0kPGVuAlvtcEOzEVmtZ55QweAyMbc= Date: Tue, 28 Jan 2025 07:55:59 -0800 From: Andre Muezerie To: David Marchand Cc: WanRenyong , dev@dpdk.org, thomas@monjalon.net, Akhil Goyal Subject: Re: [PATCH] devtool: fix falsely reporting from checkpatch Message-ID: <20250128155559.GA26333@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <20250120112654.1049456-1-wanry@yunsilicon.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Tue, Jan 28, 2025 at 04:02:27PM +0100, David Marchand wrote: > Hi, > > On Mon, Jan 20, 2025 at 12:27 PM WanRenyong wrote: > > > > When executes the check_packed_attributes function in checkpatch, > > if __rte_packed_begin or __rte_packed_end appear in the context > > of a patch file, there may be a situation where the counts of > > __rte_packed_begin and __rte_packed_end do not match, causing > > checkpatch to return a failure. > > This patch fixes this issue by only counting the lines in the > > patch file that start with a + and include either > > __rte_packed_begin or __rte_packed_end. > > > > Signed-off-by: WanRenyong > > Thanks for proposing this fix. > > When sending such fixes, don't forget to add a Fixes: and Cc: > author/maintainers by using --cc-cmd devtools/get-maintainers.sh > > > Adding Akhil, André and Thomas in the loop. > > I also had some concern about the check: > https://inbox.dpdk.org/dev/CAJFAV8w=s1L-WYk+Qv-B+Mn6eAwKrB=GTz6hU--ZoLrJsz7=DQ@mail.gmail.com/ > > But I merged the check untouched as nobody else seemed to object. > Checkpatch warnings are known to have false positive and such false > positives are "filtered" by subtree maintainers. > > > > --- > > devtools/checkpatches.sh | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh > > index 003bb49e04..2e228b7f92 100755 > > --- a/devtools/checkpatches.sh > > +++ b/devtools/checkpatches.sh > > @@ -384,8 +384,8 @@ check_packed_attributes() { # > > res=1 > > fi > > > > - begin_count=$(grep '__rte_packed_begin' "$1" | wc -l) > > - end_count=$(grep '__rte_packed_end' "$1" | wc -l) > > + begin_count=$(grep -E '^\+.*__rte_packed_begin' "$1" | wc -l) > > + end_count=$(grep -E '^\+.*__rte_packed_end' "$1" | wc -l) > > if [ $begin_count != $end_count ]; then > > echo "__rte_packed_begin and __rte_packed_end should always be used in pairs." > > res=1 > > I don't think this change is any better. > > There is a good chance that a patch touching just a first line of a > structure definition won't come along a line touching the last line of > the struct. > My suggestion (if we want to avoid those non useful warning) would be > to just remove the counting stuff in the check. > > Opinions? > > > -- > David Marchand Yes, it was known that false positives could happen, as is the case with many other checks used today. While I was working on the patchset that added this new check it helped me identify some issues with my changes. I feel I benefited from this check at that time, but if people feel that it is causing more harm than good, I am not opposed to having it removed.