From: Xining Xu <[email protected]>
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 <[email protected]>
Tested-by: Barry Song <[email protected]>
Cc: Chris Zankel <[email protected]>
Cc: Huacai Chen <[email protected]>
Cc: Herbert Xu <[email protected]>
Cc: Guenter Roeck <[email protected]>
Cc: Stephen Rothwell <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: Andy Whitcroft <[email protected]>
Cc: Dwaipayan Ray <[email protected]>
Cc: Joe Perches <[email protected]>
Cc: Jonathan Corbet <[email protected]>
Cc: Lukas Bulwahn <[email protected]>
Cc: Max Filippov <[email protected]>
---
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
On Thu, 2024-03-28 at 15:21 +1300, Barry Song wrote:
> From: Xining Xu <[email protected]>
>
> 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
On 3/27/2024 7:21 PM, Barry Song wrote:
> From: Xining Xu <[email protected]>
>
> 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 <[email protected]>
if you are re-posting somebody else's work you need to add your own Signed-off-by
> Tested-by: Barry Song <[email protected]>
> Cc: Chris Zankel <[email protected]>
> Cc: Huacai Chen <[email protected]>
> Cc: Herbert Xu <[email protected]>
> Cc: Guenter Roeck <[email protected]>
> Cc: Stephen Rothwell <[email protected]>
> Cc: Mark Brown <[email protected]>
> Cc: Andy Whitcroft <[email protected]>
> Cc: Dwaipayan Ray <[email protected]>
> Cc: Joe Perches <[email protected]>
> Cc: Jonathan Corbet <[email protected]>
> Cc: Lukas Bulwahn <[email protected]>
> Cc: Max Filippov <[email protected]>
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 <[email protected]>
> >
> > 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 <[email protected]>
>
> 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 <[email protected]>
> > Cc: Chris Zankel <[email protected]>
> > Cc: Huacai Chen <[email protected]>
> > Cc: Herbert Xu <[email protected]>
> > Cc: Guenter Roeck <[email protected]>
> > Cc: Stephen Rothwell <[email protected]>
> > Cc: Mark Brown <[email protected]>
> > Cc: Andy Whitcroft <[email protected]>
> > Cc: Dwaipayan Ray <[email protected]>
> > Cc: Joe Perches <[email protected]>
> > Cc: Jonathan Corbet <[email protected]>
> > Cc: Lukas Bulwahn <[email protected]>
> > Cc: Max Filippov <[email protected]>
>
> On Thu, 2024-03-28 at 15:21 +1300, Barry Song wrote:
> > From: Xining Xu <[email protected]>
> >
> > 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.
> On Thu, 2024-03-28 at 15:21 +1300, Barry Song wrote:
> > From: Xining Xu <[email protected]>
> >
> > 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.
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 <[email protected]>
> > >
> > > 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.
> 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 <[email protected]>
> > > >
> > > > 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