From: Barry Song <[email protected]>
-v5:
* Simplify the code for Patch 2 according to Joe's suggestions.
* add s-o-b of Barry according to Jeff Johnson
-v4:
* fix Xining's email address, s/[email protected]/[email protected]/g
* fix some false positives of checkpatch.pl
* downgrade from ERROR to WARN in checkpatch.pl
Thanks for Joe's comments!
v4 link: https://lore.kernel.org/all/[email protected]/
-v3:
https://lore.kernel.org/all/[email protected]/
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 | 6 ++++++
2 files changed, 22 insertions(+)
--
2.34.1
From: Barry Song <[email protected]>
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 <[email protected]>
Suggested-by: Max Filippov <[email protected]>
Reviewed-by: Mark Brown <[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: 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: Xining Xu <[email protected]>
---
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
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]>
Signed-off-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]>
Cc: Jeff Johnson <[email protected]>
Cc: Charlemagne Lasse <[email protected]>
---
scripts/checkpatch.pl | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 9c4c4a61bc83..9895d7e38a9f 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -6040,6 +6040,12 @@ sub process {
CHK("MACRO_ARG_PRECEDENCE",
"Macro argument '$arg' may be better as '($arg)' to avoid precedence issues\n" . "$herectx");
}
+
+# 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");
+ }
}
# check for macros with flow control, but without ## concatenation
--
2.34.1
On Mon, 2024-04-01 at 14: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.
>
> 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)
This is no longer true.
Please update the ERROR->WARN and message as below
Ideally, this would have an update to
Documentation/dev-tools/checkpatch.rst
to describe the new --verbose message type
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -6040,6 +6040,12 @@ sub process {
> CHK("MACRO_ARG_PRECEDENCE",
> "Macro argument '$arg' may be better as '($arg)' to avoid precedence issues\n" . "$herectx");
> }
> +
> +# 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");
> + }
> On Mon, 2024-04-01 at 14: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.
> >
> > 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)
>
> This is no longer true.
> Please update the ERROR->WARN and message as below
>
> Ideally, this would have an update to
> Documentation/dev-tools/checkpatch.rst
>
> to describe the new --verbose message type
Hi Joe,
Thank you for the comments, here's the code:
+# 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");
+}
and here's the document for it which is inserted into the "Macros, Attributes and
Symbols" section of checkpatch.rst starting from line 909:
+
+ **MACRO_ARG_UNUSED**
+ If function-like macros do not utilize a parameter, it might result
+ in a build warning. We advocate for utilizing static inline functions
+ to replace such macros.
+ For example, for a macro as below::
+
+ #define test(a) do { } while (0)
+
+ there would be a warning as below::
+
+ WARNING: Parameter 'a' is not used in function-like macro, please use
+ static inline instead.
Please let me know if the document needs further re-wording to make it helpful enough
to the readers.
Thanks,
Xining
On Tue, 2024-04-02 at 00:16 +0000, Mac Xu wrote:
> > On Mon, 2024-04-01 at 14: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.
> > >
> > > 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)
> >
> > This is no longer true.
> > Please update the ERROR->WARN and message as below
> >
> > Ideally, this would have an update to
> > Documentation/dev-tools/checkpatch.rst
> >
> > to describe the new --verbose message type
>
> Hi Joe,
>
> Thank you for the comments, here's the code:
>
> +# 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");
> +}
>
> and here's the document for it which is inserted into the "Macros, Attributes and
> Symbols" section of checkpatch.rst starting from line 909:
> +
> + **MACRO_ARG_UNUSED**
> + If function-like macros do not utilize a parameter, it might result
> + in a build warning. We advocate for utilizing static inline functions
> + to replace such macros.
> + For example, for a macro as below::
> +
> + #define test(a) do { } while (0)
> +
> + there would be a warning as below::
> +
> + WARNING: Parameter 'a' is not used in function-like macro, please use
> + static inline instead.
>
> Please let me know if the document needs further re-wording to make it helpful enough
> to the readers.
Hi again Xining.
Thanks.
That looks good but it doesn't match the script output
which doesn't use ", please use static inline instead."
(and I believe the script should not output that too)
Another good thing would be to add a line like:
See: https://www.kernel.org/doc/html/latest/process/coding-style.html#macros-enums-and-rtl
For example, from: checkpatch.rst
**ALLOC_SIZEOF_STRUCT**
The allocation style is bad. In general for family of
allocation functions using sizeof() to get memory size,
constructs like::
p = alloc(sizeof(struct foo), ...)
should be::
p = alloc(sizeof(*p), ...)
See: https://www.kernel.org/doc/html/latest/process/coding-style.html#allocating-memory
> On Tue, 2024-04-02 at 00:16 +0000, Mac Xu wrote:
> > > On Mon, 2024-04-01 at 14: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.
> > > >
> > > > 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)
> > >
> > > This is no longer true.
> > > Please update the ERROR->WARN and message as below
> > >
> > > Ideally, this would have an update to
> > > Documentation/dev-tools/checkpatch.rst
> > >
> > > to describe the new --verbose message type
> >
> > Hi Joe,
> >
> > Thank you for the comments, here's the code:
> >
> > +# 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");
> > +}
> >
> > and here's the document for it which is inserted into the "Macros, Attributes and
> > Symbols" section of checkpatch.rst starting from line 909:
> > +
> > + **MACRO_ARG_UNUSED**
> > + If function-like macros do not utilize a parameter, it might result
> > + in a build warning. We advocate for utilizing static inline functions
> > + to replace such macros.
> > + For example, for a macro as below::
> > +
> > + #define test(a) do { } while (0)
> > +
> > + there would be a warning as below::
> > +
> > + WARNING: Parameter 'a' is not used in function-like macro, please use
> > + static inline instead.
> >
> > Please let me know if the document needs further re-wording to make it helpful enough
> > to the readers.
>
> Hi again Xining.
>
> Thanks.
>
> That looks good but it doesn't match the script output
> which doesn't use ", please use static inline instead."
> (and I believe the script should not output that too)
>
> Another good thing would be to add a line like:
>
> See: https://www.kernel.org/doc/html/latest/process/coding-style.html#macros-enums-and-rtl
>
> For example, from: checkpatch.rst
>
> **ALLOC_SIZEOF_STRUCT**
> The allocation style is bad. In general for family of
> allocation functions using sizeof() to get memory size,
> constructs like::
>
> p = alloc(sizeof(struct foo), ...)
>
> should be::
>
> p = alloc(sizeof(*p), ...)
>
> See: https://www.kernel.org/doc/html/latest/process/coding-style.html#allocating-memory
>
>
Hi again Joe,
Thanks again for the detailed comments.
Here's the code (which keeps unchanged):
+# 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");
+}
And here's the updated document, which drops the ", please use static inline instead" from the
warning message, and adds a link at the end of this document block:
+
+ **MACRO_ARG_UNUSED**
+ If function-like macros do not utilize a parameter, it might result
+ in a build warning. We advocate for utilizing static inline functions
+ to replace such macros.
+ For example, for a macro such as the one below::
+
+ #define test(a) do { } while (0)
+
+ there would be a warning like below::
+
+ WARNING: Parameter 'a' is not used in function-like macro.
+
+ See: https://www.kernel.org/doc/html/latest/process/coding-style.html#macros-enums-and-rtl
Regards,
Xining.
So I'm not sure what your desired path for getting this upstream is. I
can take it, but I'm generally quite leery of taking coding-style
changes without some serious acks on them - nobody elected me as the
arbiter of proper coding style.
A nit below
Barry Song <[email protected]> writes:
> From: Barry Song <[email protected]>
>
> 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 <[email protected]>
> Suggested-by: Max Filippov <[email protected]>
> Reviewed-by: Mark Brown <[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: 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: Xining Xu <[email protected]>
> ---
> 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
I would just use the "::" notation here; the ..code-block:: just adds
noise IMO.
> + 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)
> +
1) If you're putting in examples of something *not* to do, it's probably
better to also put in something like:
/* don't do this */
people don't always read closely.
2) Can we say *why* it's not recommended?
Thanks,
jon
On Wed, Apr 3, 2024 at 5:13 AM Jonathan Corbet <[email protected]> wrote:
>
> So I'm not sure what your desired path for getting this upstream is. I
> can take it, but I'm generally quite leery of taking coding-style
> changes without some serious acks on them - nobody elected me as the
> arbiter of proper coding style.
Hi Jonathan,
Thanks!
Andrew previously integrated it into mm-nomm and tagged it as [TO-BE-UPDATED]
before removing it a few days back. Here's the link:
https://lore.kernel.org/mm-commits/[email protected]/
So if feasible, I'd prefer to stick with Andrew's channel.
>
> A nit below
>
> Barry Song <[email protected]> writes:
>
> > From: Barry Song <[email protected]>
> >
> > 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 <[email protected]>
> > Suggested-by: Max Filippov <[email protected]>
> > Reviewed-by: Mark Brown <[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: 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: Xining Xu <[email protected]>
> > ---
> > 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
>
> I would just use the "::" notation here; the ..code-block:: just adds
> noise IMO.
I am not quite sure we want this. as reading the whole coding-style.rst,
. code-block:: c is everywhere :-) Should I do something different or
just follow the tradition?
>
> > + 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)
> > +
>
> 1) If you're putting in examples of something *not* to do, it's probably
> better to also put in something like:
>
> /* don't do this */
>
> people don't always read closely.
ok.
>
> 2) Can we say *why* it's not recommended?
>
Andrew makes a valid point.
https://lore.kernel.org/all/[email protected]/
"I think so. My overall view is that we should write things in C. Only
use macros if the thing we're trying to do simply cannot be done in a C
function.
- inline functions don't have the "expression with side effects
evaluated more than once" problem.
- inline functions avoid the unused-variable issue which started this thread
- inline functions look better
- for some reason, people are more inclined to document inline
functions than macros."
Andrew's point seems too lengthy for inclusion in the coding-style.rst document?
I'll attempt to condense it.
> Thanks,
>
> jon
Thanks
Barry
On Wed, 2024-04-03 at 10:21 +1300, Barry Song wrote:
> On Wed, Apr 3, 2024 at 5:13 AM Jonathan Corbet <[email protected]> wrote:
> >
> > So I'm not sure what your desired path for getting this upstream is. I
> > can take it, but I'm generally quite leery of taking coding-style
> > changes without some serious acks on them - nobody elected me as the
> > arbiter of proper coding style.
I believe it is generally appropriate for macros that take
arguments to use static inlines where feasible, so:
Acked-by: Joe Perches <[email protected]>
And yes, mm is the usual path for upstreaming at least this
sort of checkpatch change.