Re: [PATCH] devtool: fix falsely reporting from checkpatch

[Andre Muezerie <andremue@linux.microsoft.com>]

On Tue, Jan 28, 2025 at 04:02:27PM +0100, David Marchand wrote:
> Hi,
> 
> On Mon, Jan 20, 2025 at 12:27 PM WanRenyong <wanry@yunsilicon.com> 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 <wanry@yunsilicon.com>
> 
> 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() { # <patch>
> >                 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.