workflows.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 0/2] codingstyle: avoid unused parameters for a function-like macro
@ 2024-03-28  2:21 Barry Song
  2024-03-28  2:21 ` [PATCH v4 1/2] Documentation: coding-style: ask function-like macros to evaluate parameters Barry Song
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Barry Song @ 2024-03-28  2:21 UTC (permalink / raw)
  To: akpm, linux-doc
  Cc: apw, broonie, chenhuacai, chris, corbet, dwaipayanray1, herbert,
	joe, linux-kernel, linux, lukas.bulwahn, mac.xxn, sfr,
	v-songbaohua, workflows

From: Barry Song <v-songbaohua@oppo.com>

-v4:
 * fix Xining's email address, s/ma.xxn@outlook.com/mac.xxn@outlook.com/g
 * fix some false positives of checkpatch.pl
 * downgrade from ERROR to WARN in checkpatch.pl

 Thanks for Joe's comments!

-v3:
 https://lore.kernel.org/all/20240322084937.66018-1-21cnbao@gmail.com/

A function-like macro could result in build warnings such as
"unused variable." This patchset updates the guidance to
recommend always using a static inline function instead
and also provides checkpatch support for this new rule.

Barry Song (1):
  Documentation: coding-style: ask function-like macros to evaluate
    parameters

Xining Xu (1):
  scripts: checkpatch: check unused parameters for function-like macro

 Documentation/process/coding-style.rst | 16 ++++++++++++++
 scripts/checkpatch.pl                  | 30 ++++++++++++++++++++++++++
 2 files changed, 46 insertions(+)

-- 
2.34.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v4 1/2] Documentation: coding-style: ask function-like macros to evaluate parameters
  2024-03-28  2:21 [PATCH v4 0/2] codingstyle: avoid unused parameters for a function-like macro Barry Song
@ 2024-03-28  2:21 ` Barry Song
  2024-03-28  2:21 ` [PATCH v4 2/2] scripts: checkpatch: check unused parameters for function-like macro Barry Song
  2024-03-28  4:22 ` [PATCH v4 0/2] codingstyle: avoid unused parameters for a " Barry Song
  2 siblings, 0 replies; 11+ messages in thread
From: Barry Song @ 2024-03-28  2:21 UTC (permalink / raw)
  To: akpm, linux-doc
  Cc: apw, broonie, chenhuacai, chris, corbet, dwaipayanray1, herbert,
	joe, linux-kernel, linux, lukas.bulwahn, mac.xxn, sfr,
	v-songbaohua, workflows, Max Filippov

From: Barry Song <v-songbaohua@oppo.com>

Recent commit 77292bb8ca69c80 ("crypto: scomp - remove memcpy if
sg_nents is 1 and pages are lowmem") leads to warnings on xtensa
and loongarch,
   In file included from crypto/scompress.c:12:
   include/crypto/scatterwalk.h: In function 'scatterwalk_pagedone':
   include/crypto/scatterwalk.h:76:30: warning: variable 'page' set but not used [-Wunused-but-set-variable]
      76 |                 struct page *page;
         |                              ^~~~
   crypto/scompress.c: In function 'scomp_acomp_comp_decomp':
>> crypto/scompress.c:174:38: warning: unused variable 'dst_page' [-Wunused-variable]
     174 |                         struct page *dst_page = sg_page(req->dst);
         |

The reason is that flush_dcache_page() is implemented as a noop
macro on these platforms as below,

 #define flush_dcache_page(page) do { } while (0)

The driver code, for itself, seems be quite innocent and placing
maybe_unused seems pointless,

 struct page *dst_page = sg_page(req->dst);

 for (i = 0; i < nr_pages; i++)
 	flush_dcache_page(dst_page + i);

And it should be independent of architectural implementation
differences.

Let's provide guidance on coding style for requesting parameter
evaluation or proposing the migration to a static inline
function.

Signed-off-by: Barry Song <v-songbaohua@oppo.com>
Suggested-by: Max Filippov <jcmvbkbc@gmail.com>
Reviewed-by: Mark Brown <broonie@kernel.org>
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: 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: Xining Xu <mac.xxn@outlook.com>
---
 Documentation/process/coding-style.rst | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
index 9c7cf7347394..791d333a57fd 100644
--- a/Documentation/process/coding-style.rst
+++ b/Documentation/process/coding-style.rst
@@ -827,6 +827,22 @@ Macros with multiple statements should be enclosed in a do - while block:
 				do_this(b, c);		\
 		} while (0)
 
+Function-like macros with unused parameters should be replaced by static
+inline functions to avoid the issue of unused variables:
+
+.. code-block:: c
+
+	static inline void fun(struct foo *foo)
+	{
+	}
+
+For historical reasons, many files still use the cast to (void) to evaluate
+parameters, but this method is not recommended:
+
+.. code-block:: c
+
+	#define macrofun(foo) do { (void) (foo); } while (0)
+
 Things to avoid when using macros:
 
 1) macros that affect control flow:
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v4 2/2] scripts: checkpatch: check unused parameters for function-like macro
  2024-03-28  2:21 [PATCH v4 0/2] codingstyle: avoid unused parameters for a function-like macro Barry Song
  2024-03-28  2:21 ` [PATCH v4 1/2] Documentation: coding-style: ask function-like macros to evaluate parameters Barry Song
@ 2024-03-28  2:21 ` Barry Song
  2024-03-28  9:57   ` Joe Perches
  2024-03-28 16:01   ` Jeff Johnson
  2024-03-28  4:22 ` [PATCH v4 0/2] codingstyle: avoid unused parameters for a " Barry Song
  2 siblings, 2 replies; 11+ messages in thread
From: Barry Song @ 2024-03-28  2:21 UTC (permalink / raw)
  To: akpm, linux-doc
  Cc: apw, broonie, chenhuacai, chris, corbet, dwaipayanray1, herbert,
	joe, linux-kernel, linux, lukas.bulwahn, mac.xxn, sfr,
	v-songbaohua, workflows, Max Filippov

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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 0/2] codingstyle: avoid unused parameters for a function-like macro
  2024-03-28  2:21 [PATCH v4 0/2] codingstyle: avoid unused parameters for a function-like macro Barry Song
  2024-03-28  2:21 ` [PATCH v4 1/2] Documentation: coding-style: ask function-like macros to evaluate parameters Barry Song
  2024-03-28  2:21 ` [PATCH v4 2/2] scripts: checkpatch: check unused parameters for function-like macro Barry Song
@ 2024-03-28  4:22 ` Barry Song
  2 siblings, 0 replies; 11+ messages in thread
From: Barry Song @ 2024-03-28  4:22 UTC (permalink / raw)
  To: akpm, mac.xxn
  Cc: apw, broonie, chenhuacai, chris, corbet, dwaipayanray1, herbert,
	joe, linux-kernel, linux, lukas.bulwahn, sfr, v-songbaohua,
	workflows, linux-doc

On Thu, Mar 28, 2024 at 3:21 PM Barry Song <21cnbao@gmail.com> wrote:
>
> From: Barry Song <v-songbaohua@oppo.com>
>
> -v4:
>  * fix Xining's email address, s/ma.xxn@outlook.com/mac.xxn@outlook.com/g

Hi Andrew,

Apologies for the oversight. Could you please apply these two patches to replace
the ones in the mm-nonmm-unstable branch? We need to correct Xining's email
address regardless.

>  * fix some false positives of checkpatch.pl
>  * downgrade from ERROR to WARN in checkpatch.pl
>
>  Thanks for Joe's comments!
>
> -v3:
>  https://lore.kernel.org/all/20240322084937.66018-1-21cnbao@gmail.com/
>
> A function-like macro could result in build warnings such as
> "unused variable." This patchset updates the guidance to
> recommend always using a static inline function instead
> and also provides checkpatch support for this new rule.
>
> Barry Song (1):
>   Documentation: coding-style: ask function-like macros to evaluate
>     parameters
>
> Xining Xu (1):
>   scripts: checkpatch: check unused parameters for function-like macro
>
>  Documentation/process/coding-style.rst | 16 ++++++++++++++
>  scripts/checkpatch.pl                  | 30 ++++++++++++++++++++++++++
>  2 files changed, 46 insertions(+)
>
> --
> 2.34.1
>

Thanks
Barry

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 2/2] scripts: checkpatch: check unused parameters for function-like macro
  2024-03-28  2:21 ` [PATCH v4 2/2] scripts: checkpatch: check unused parameters for function-like macro Barry Song
@ 2024-03-28  9:57   ` Joe Perches
  2024-03-31 13:43     ` Mac Xu
  2024-03-31 13:46     ` Mac Xu
  2024-03-28 16:01   ` Jeff Johnson
  1 sibling, 2 replies; 11+ messages in thread
From: Joe Perches @ 2024-03-28  9:57 UTC (permalink / raw)
  To: Barry Song, akpm, linux-doc
  Cc: apw, broonie, chenhuacai, chris, corbet, dwaipayanray1, herbert,
	linux-kernel, linux, lukas.bulwahn, mac.xxn, sfr, v-songbaohua,
	workflows, Max Filippov

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 2/2] scripts: checkpatch: check unused parameters for function-like macro
  2024-03-28  2:21 ` [PATCH v4 2/2] scripts: checkpatch: check unused parameters for function-like macro Barry Song
  2024-03-28  9:57   ` Joe Perches
@ 2024-03-28 16:01   ` Jeff Johnson
  2024-03-28 21:24     ` Barry Song
  1 sibling, 1 reply; 11+ messages in thread
From: Jeff Johnson @ 2024-03-28 16:01 UTC (permalink / raw)
  To: Barry Song, akpm, linux-doc
  Cc: apw, broonie, chenhuacai, chris, corbet, dwaipayanray1, herbert,
	joe, linux-kernel, linux, lukas.bulwahn, mac.xxn, sfr,
	v-songbaohua, workflows, Max Filippov

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>


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 2/2] scripts: checkpatch: check unused parameters for function-like macro
  2024-03-28 16:01   ` Jeff Johnson
@ 2024-03-28 21:24     ` Barry Song
  0 siblings, 0 replies; 11+ messages in thread
From: Barry Song @ 2024-03-28 21:24 UTC (permalink / raw)
  To: Jeff Johnson
  Cc: akpm, linux-doc, apw, broonie, chenhuacai, chris, corbet,
	dwaipayanray1, herbert, joe, linux-kernel, linux, lukas.bulwahn,
	mac.xxn, sfr, v-songbaohua, workflows, Max Filippov

On Fri, Mar 29, 2024 at 5:01 AM Jeff Johnson <quic_jjohnson@quicinc.com> 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>
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 2/2] scripts: checkpatch: check unused parameters for function-like macro
  2024-03-28  9:57   ` Joe Perches
@ 2024-03-31 13:43     ` Mac Xu
  2024-03-31 13:46     ` Mac Xu
  1 sibling, 0 replies; 11+ messages in thread
From: Mac Xu @ 2024-03-31 13:43 UTC (permalink / raw)
  To: Joe Perches, Barry Song, akpm, linux-doc
  Cc: apw, broonie, chenhuacai, chris, corbet, dwaipayanray1, herbert,
	linux-kernel, linux, lukas.bulwahn, sfr, v-songbaohua, workflows,
	Max Filippov


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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 2/2] scripts: checkpatch: check unused parameters for function-like macro
  2024-03-28  9:57   ` Joe Perches
  2024-03-31 13:43     ` Mac Xu
@ 2024-03-31 13:46     ` Mac Xu
  2024-03-31 15:54       ` Joe Perches
  1 sibling, 1 reply; 11+ messages in thread
From: Mac Xu @ 2024-03-31 13:46 UTC (permalink / raw)
  To: Joe Perches, Barry Song, akpm, linux-doc
  Cc: apw, broonie, chenhuacai, chris, corbet, dwaipayanray1, herbert,
	linux-kernel, linux, lukas.bulwahn, sfr, v-songbaohua, workflows,
	Max Filippov

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 2/2] scripts: checkpatch: check unused parameters for function-like macro
  2024-03-31 13:46     ` Mac Xu
@ 2024-03-31 15:54       ` Joe Perches
  2024-03-31 23:21         ` Mac Xu
  0 siblings, 1 reply; 11+ messages in thread
From: Joe Perches @ 2024-03-31 15:54 UTC (permalink / raw)
  To: Mac Xu, Barry Song, akpm, linux-doc
  Cc: apw, broonie, chenhuacai, chris, corbet, dwaipayanray1, herbert,
	linux-kernel, linux, lukas.bulwahn, sfr, v-songbaohua, workflows,
	Max Filippov

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.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 2/2] scripts: checkpatch: check unused parameters for function-like macro
  2024-03-31 15:54       ` Joe Perches
@ 2024-03-31 23:21         ` Mac Xu
  0 siblings, 0 replies; 11+ messages in thread
From: Mac Xu @ 2024-03-31 23:21 UTC (permalink / raw)
  To: Joe Perches, Barry Song, akpm, linux-doc
  Cc: apw, broonie, chenhuacai, chris, corbet, dwaipayanray1, herbert,
	linux-kernel, linux, lukas.bulwahn, sfr, v-songbaohua, workflows,
	Max Filippov


> 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2024-03-31 23:21 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-28  2:21 [PATCH v4 0/2] codingstyle: avoid unused parameters for a function-like macro Barry Song
2024-03-28  2:21 ` [PATCH v4 1/2] Documentation: coding-style: ask function-like macros to evaluate parameters Barry Song
2024-03-28  2:21 ` [PATCH v4 2/2] scripts: checkpatch: check unused parameters for function-like macro Barry Song
2024-03-28  9:57   ` Joe Perches
2024-03-31 13:43     ` Mac Xu
2024-03-31 13:46     ` Mac Xu
2024-03-31 15:54       ` Joe Perches
2024-03-31 23:21         ` Mac Xu
2024-03-28 16:01   ` Jeff Johnson
2024-03-28 21:24     ` Barry Song
2024-03-28  4:22 ` [PATCH v4 0/2] codingstyle: avoid unused parameters for a " Barry Song

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).