On Tue, Jan 28, 2025 at 04:02:27PM +0100, David Marchand wrote: > Hi, > > On Mon, Jan 20, 2025 at 12:27 PM WanRenyong <wa...@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 <wa...@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=d...@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.
