Re: [PATCH v4 2/2] scripts: checkpatch: check unused parameters for function-like macro

[Mac Xu]

> On Thu, 2024-03-28 at 15:21 +1300, Barry Song wrote:
> > From: Xining Xu <mac.xxn@xxxxxxxxxxx>
> >
> > If function-like macros do not utilize a parameter, it might result in a
> > build warning.  In our coding style guidelines, we advocate for utilizing
> > static inline functions to replace such macros.  This patch verifies
> > compliance with the new rule.
> []
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > @@ -6109,6 +6109,36 @@ sub process {
> >                                WARN("TRAILING_SEMICOLON",
> >                                     "macros should not use a trailing semicolon\n" . "$herectx");
> >                        }
> > +
> > +                     # match "\s*" rather than "\s+" after the balanced parens, as macro definition with arguments
> > +                     # is not required to have whitespace after arguments
> > +                     if ($dstat =~ /^\+\s*#\s*define\s+$Ident$balanced_parens\s*(\S+.*)(\/[\/*].*)?/) {
> 
> I think '(\/[\/*].*)?' doesn't do what you expect
> perhaps '(\/[\/\*].*)?'
> though I don't know why this should be capture group

I'd wanted to capture the comment to handle a case where a unused param happens to appears in a comment

> 
> > +                             my $params = $1 || "";
> 
> 
> > +                             my $body = $2 || "";
> 
> Should never get the || "" as the 2nd capture group is not optional
> 
> > +
> > +                         # get the individual params
> > +                             $params =~ tr/()//d;
> > +                             # remove leading and trailing whitespace
> > +                             $params =~ s/^\s+|\s+$//g;
> > +
> > +                             $ctx =~ s/\n*$//;
> > +                             my $cnt = statement_rawlines($ctx);
> > +                             my $herectx = get_stat_here($linenr, $cnt, $here);
> > +
> > +                             if ($params ne "") {
> 
> probably unnecessary
> 
> > +                                     my @paramList = split /,\s*/, $params;
> 
> please use split() with parentheses
> 
> > +                                     foreach my $param(@paramList) {
> 
> maybe
>                                 foreach my $param (split(/,/, $params) {
>                                         $param = trim($param);
>                                         next if ($param =~ /\.\.\.$/);
> > +                                             if ($param =~ /\.\.\.$/) {
> > +                                                     # if the param name ends with "...", skip the check
> > +                                                     next;
> > +                                             }
> > +                                             if ($body !~ /\b$param\b/) {
> > +                                                     WARN("UNUSED_PARAM_IN_MACRO",
> > +                                                             "Parameter '$param' is not used in function-like macro\n" . "$herectx");
> > +                                             }
> > +                                     }
> It seems this logic is a bit redundant to existing
> code and might be better added in the block that starts
> 
> (line 6026)
> # check if any macro arguments are reused (ignore '...' and 'type')
> 
> as that already does each param in a #define and
> ignores ... and type

Hi Joe,

Thank you for your comments with insights, as you said, code block of line 6026 is a better place to 
place this new logic, as it already handles the logic I'd wanted like extracting, splitting and trimming 
the arguments, excluding the trailing comments etc.

By placing the logic in the new place, code duplicates are reduced.

Here's my new code (inserted from line 6044):
+# check if this is an unused argument
+        if ($define_stmt !~ /\b$arg\b/) {
+                WARN("UNUSED_ARG_IN_MACRO",
+                        "Argument '$arg' is not used in function-like macro\n" . "$herectx");
+        }
+}

Please note that I use the wording of "arg/argument" instead of "param/parameter" for consistency, please let me know if if this is the 
correct wording to use here.


Thanks,
Mac.