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

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

Mar 27, 2024, 7:21 PM

Post #1 of 8 (33 views)

From: Xining Xu <mac.xxn@outlook.com>

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.

For a macro such as the one below,

#define test(a) do { } while (0)

The test result is as follows.

ERROR: Parameter 'a' is not used in function-like macro, please use static
inline instead
#21: FILE: mm/init-mm.c:20:
+#define test(a) do { } while (0)

total: 1 errors, 0 warnings, 8 lines checked

Signed-off-by: Xining Xu <mac.xxn@outlook.com>
Tested-by: Barry Song <v-songbaohua@oppo.com>
Cc: Chris Zankel <chris@zankel.net>
Cc: Huacai Chen <chenhuacai@loongson.cn>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Stephen Rothwell <sfr@canb.auug.org.au>
Cc: Mark Brown <broonie@kernel.org>
Cc: Andy Whitcroft <apw@canonical.com>
Cc: Dwaipayan Ray <dwaipayanray1@gmail.com>
Cc: Joe Perches <joe@perches.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Cc: Max Filippov <jcmvbkbc@gmail.com>
---
scripts/checkpatch.pl | 30 ++++++++++++++++++++++++++++++
1 file changed, 30 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 9c4c4a61bc83..bcb886014d60 100755
--- 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+.*)(\/[\/*].*)?/) {
+ my $params = $1 || "";
+ my $body = $2 || "";
+
+ # 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 "") {
+ my @paramList = split /,\s*/, $params;
+ foreach my $param(@paramList) {
+ 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");
+ }
+ }
+ }
+ }
}

# check for redundant bracing round if etc
--
2.34.1

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

joe at perches

Mar 28, 2024, 2:57 AM

Post #2 of 8 (33 views)

Permalink

On Thu, 2024-03-28 at 15:21 +1300, Barry Song wrote:
> From: Xining Xu <mac.xxn@outlook.com>
>
> 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

> + 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

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

quic_jjohnson at quicinc

Mar 28, 2024, 9:01 AM

Post #3 of 8 (33 views)

Permalink

On 3/27/2024 7:21 PM, Barry Song wrote:
> From: Xining Xu <mac.xxn@outlook.com>
>
> 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.
>
> For a macro such as the one below,
>
> #define test(a) do { } while (0)
>
> The test result is as follows.
>
> ERROR: Parameter 'a' is not used in function-like macro, please use static
> inline instead
> #21: FILE: mm/init-mm.c:20:
> +#define test(a) do { } while (0)
>
> total: 1 errors, 0 warnings, 8 lines checked
>
> Signed-off-by: Xining Xu <mac.xxn@outlook.com>

if you are re-posting somebody else's work you need to add your own Signed-off-by

> Tested-by: Barry Song <v-songbaohua@oppo.com>
> Cc: Chris Zankel <chris@zankel.net>
> Cc: Huacai Chen <chenhuacai@loongson.cn>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Stephen Rothwell <sfr@canb.auug.org.au>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Andy Whitcroft <apw@canonical.com>
> Cc: Dwaipayan Ray <dwaipayanray1@gmail.com>
> Cc: Joe Perches <joe@perches.com>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> Cc: Max Filippov <jcmvbkbc@gmail.com>

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

21cnbao at gmail

Mar 28, 2024, 2:24 PM

Post #4 of 8 (33 views)

Permalink

On Fri, Mar 29, 2024 at 5:01?AM Jeff Johnson <quic_jjohnson@quicinccom> wrote:
>
> On 3/27/2024 7:21 PM, Barry Song wrote:
> > From: Xining Xu <mac.xxn@outlook.com>
> >
> > 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.
> >
> > For a macro such as the one below,
> >
> > #define test(a) do { } while (0)
> >
> > The test result is as follows.
> >
> > ERROR: Parameter 'a' is not used in function-like macro, please use static
> > inline instead
> > #21: FILE: mm/init-mm.c:20:
> > +#define test(a) do { } while (0)
> >
> > total: 1 errors, 0 warnings, 8 lines checked
> >
> > Signed-off-by: Xining Xu <mac.xxn@outlook.com>
>
> if you are re-posting somebody else's work you need to add your own Signed-off-by

Ok. Jeff, I will do it in the new version and obviously Joe still has
some remaining
comments to be addressed by Xining.

>
> > Tested-by: Barry Song <v-songbaohua@oppo.com>
> > Cc: Chris Zankel <chris@zankel.net>
> > Cc: Huacai Chen <chenhuacai@loongson.cn>
> > Cc: Herbert Xu <herbert@gondor.apana.org.au>
> > Cc: Guenter Roeck <linux@roeck-us.net>
> > Cc: Stephen Rothwell <sfr@canb.auug.org.au>
> > Cc: Mark Brown <broonie@kernel.org>
> > Cc: Andy Whitcroft <apw@canonical.com>
> > Cc: Dwaipayan Ray <dwaipayanray1@gmail.com>
> > Cc: Joe Perches <joe@perches.com>
> > Cc: Jonathan Corbet <corbet@lwn.net>
> > Cc: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> > Cc: Max Filippov <jcmvbkbc@gmail.com>
>

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

mac.xxn at outlook

Mar 31, 2024, 6:43 AM

Post #5 of 8 (33 views)

Permalink

> On Thu, 2024-03-28 at 15:21 +1300, Barry Song wrote:
> > From: Xining Xu <mac.xxn@outlook.com>
> >
> > 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 drastically 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.

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

mac.xxn at outlook

Mar 31, 2024, 6:46 AM

Post #6 of 8 (33 views)

Permalink

> On Thu, 2024-03-28 at 15:21 +1300, Barry Song wrote:
> > From: Xining Xu <mac.xxn@outlook.com>
> >
> > 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.

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

joe at perches

Mar 31, 2024, 8:54 AM

Post #7 of 8 (31 views)

Permalink

On Sun, 2024-03-31 at 13:46 +0000, Mac Xu wrote:
> > On Thu, 2024-03-28 at 15:21 +1300, Barry Song wrote:
> > > From: Xining Xu <mac.xxn@outlook.com>
> > >
> > > 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
>
[]
> > 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",

Perhaps
WARN("MACRO_ARG_UNUSED",
...

to better match the others above it in the block:

CHK("MACRO_ARG_REUSE",
and
CHK("MACRO_ARG_PRECEDENCE",

Other than that trivial bit, seems ok.

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

mac.xxn at outlook

Mar 31, 2024, 4:21 PM

Post #8 of 8 (31 views)

Permalink

> On Sun, 2024-03-31 at 13:46 +0000, Mac Xu wrote:
> > > On Thu, 2024-03-28 at 15:21 +1300, Barry Song wrote:
> > > > From: Xining Xu <mac.xxn@outlook.com>
> > > >
> > > > 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
> >
> []
> > > 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",
> Perhaps
> WARN("MACRO_ARG_UNUSED",
> ...
>
> to better match the others above it in the block:
>
> CHK("MACRO_ARG_REUSE",
> and
> CHK("MACRO_ARG_PRECEDENCE",
>
> Other than that trivial bit, seems ok.

Sure, updated, thank you!

+# check if this is an unused argument
+if ($define_stmt !~ /\b$arg\b/) {
+ WARN("MACRO_ARG_UNUSED",
+ "Argument '$arg' is not used in function-like macro\n" . "$herectx");
+}

Regards,
Xining