Hi,
I think your check for SINGLE_STATEMENT_DO_WHILE_MACRO is wrong. Just to give
an example:
#define foobar(x) \
do { \
if (pizza_ready(x)) \
eat_pizza(x); \
} while (0)
if (hungry(y))
foobar(x);
else
barfoo(x);
checkpatch does now complain about something like "WARNING: Single statement
macros should not use a do {} while (0) loop"
But we would have an ambiguous else when the do-while is removed. The code
works as expected with the do-while but the else is "attached" to the wrong
"if" when the do-while is removed.
And yes, this example is made that easy to make it easy to understand. There
are examples were static inline code would not work very well (vararg for
example).
Please fix or remove your check. Otherwise some people will be start to
overeagerly change these macros and break the kernel doing that.
--
Franz Schrober
On Thu, Aug 02, 2012 at 10:00:04AM +0200, Schrober wrote:
> Hi,
>
> I think your check for SINGLE_STATEMENT_DO_WHILE_MACRO is wrong. Just to give
> an example:
>
> #define foobar(x) \
> do { \
> if (pizza_ready(x)) \
> eat_pizza(x); \
> } while (0)
>
>
> if (hungry(y))
> foobar(x);
> else
> barfoo(x);
>
> checkpatch does now complain about something like "WARNING: Single statement
> macros should not use a do {} while (0) loop"
>
> But we would have an ambiguous else when the do-while is removed. The code
> works as expected with the do-while but the else is "attached" to the wrong
> "if" when the do-while is removed.
>
> And yes, this example is made that easy to make it easy to understand. There
> are examples were static inline code would not work very well (vararg for
> example).
>
> Please fix or remove your check. Otherwise some people will be start to
> overeagerly change these macros and break the kernel doing that.
It does appear this check should not apply when a control statement is
included.
-apw
commit b13edf7ff2d ("checkpatch: add checks for do {} while (0) macro misuses")
added a test that is overly simplistic for single statement macros.
Macros that start with control tests should be enclosed
in a do {} while (0) loop.
Add the necessary control tests to the check.
Signed-off-by: Joe Perches <[email protected]>
---
On Thu, 2012-08-02 at 15:20 +0100, Andy Whitcroft wrote:
> On Thu, Aug 02, 2012 at 10:00:04AM +0200, Schrober wrote:
> > Hi,
Hello and thanks for the report.
> > I think your check for SINGLE_STATEMENT_DO_WHILE_MACRO is wrong.
And you are correct.
> It does appear this check should not apply when a control statement is
> included.
The patch below is tested with:
$ cat t.c
#define bar_1(foo) \
if (foo) \
baz(); \
#define bar_2(foo) \
do { \
if (foo) \
baz(); \
} while (0)
#define bar_3(foo) \
do { \
baz(); \
} while (0)
#define bar_4(foo) \
do { \
baz(); \
} while (0);
#define bar_5(foo) \
do { \
while (foo) \
baz(); \
} while (0)
#define bar_6(foo) \
do { \
switch (foo) { \
case 1: \
baz(); \
} \
} while (0)
$
So perhaps...
scripts/checkpatch.pl | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 913d6bd..ca05ba2 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3016,7 +3016,8 @@ sub process {
$herectx .= raw_line($linenr, $n) . "\n";
}
- if (($stmts =~ tr/;/;/) == 1) {
+ if (($stmts =~ tr/;/;/) == 1 &&
+ $stmts !~ /^\s*(if|while|for|switch)\b/) {
WARN("SINGLE_STATEMENT_DO_WHILE_MACRO",
"Single statement macros should not use a do {} while (0) loop\n" . "$herectx");
}
On Thu, Aug 02, 2012 at 08:26:53AM -0700, Joe Perches wrote:
> The patch below is tested with:
> $ cat t.c
>
> #define bar_1(foo) \
> if (foo) \
> baz(); \
>
> #define bar_2(foo) \
> do { \
> if (foo) \
> baz(); \
> } while (0)
>
> #define bar_3(foo) \
> do { \
> baz(); \
> } while (0)
>
> #define bar_4(foo) \
> do { \
> baz(); \
> } while (0);
>
> #define bar_5(foo) \
> do { \
> while (foo) \
> baz(); \
> } while (0)
>
> #define bar_6(foo) \
> do { \
> switch (foo) { \
> case 1: \
> baz(); \
> } \
> } while (0)
>
> $
>
> So perhaps...
>
> scripts/checkpatch.pl | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 913d6bd..ca05ba2 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -3016,7 +3016,8 @@ sub process {
> $herectx .= raw_line($linenr, $n) . "\n";
> }
>
> - if (($stmts =~ tr/;/;/) == 1) {
> + if (($stmts =~ tr/;/;/) == 1 &&
> + $stmts !~ /^\s*(if|while|for|switch)\b/) {
> WARN("SINGLE_STATEMENT_DO_WHILE_MACRO",
> "Single statement macros should not use a do {} while (0) loop\n" . "$herectx");
> }
>
>
Looks reasonable enough.
Acked-by: Andy Whitcroft <[email protected]>
-apw
On Thursday 02 August 2012 17:23:22 Andy Whitcroft wrote:
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 913d6bd..ca05ba2 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -3016,7 +3016,8 @@ sub process {
> >
> > $herectx .= raw_line($linenr, $n) . "\n";
> >
> > }
> >
> > - if (($stmts =~ tr/;/;/) == 1) {
> > + if (($stmts =~ tr/;/;/) == 1 &&
> > + $stmts !~ /^\s*(if|while|for|switch)\b/) {
> >
> > WARN("SINGLE_STATEMENT_DO_WHILE_MACRO",
> >
> > "Single statement macros should not use a do {} while
(0)
> > loop\n" . "$herectx");>
> > }
>
> Looks reasonable enough.
>
> Acked-by: Andy Whitcroft <[email protected]>
Works well for me, thanks
Just as note: I think this test was accidently added twice. Once in commit
b13edf7ff2dd0fef95e981170fa71fa6b60421b0 and one time in
7d64ef43914a734990e48a80bc60686c8db21946 (just checked linux-next, and Stephen
added it again). Maybe Andrew Morton's tree is out of sync.
--
Franz Schrober
Hi Franz,
On Thu, 02 Aug 2012 18:44:57 +0200 Schrober <[email protected]> wrote:
>
> On Thursday 02 August 2012 17:23:22 Andy Whitcroft wrote:
> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > > index 913d6bd..ca05ba2 100755
> > > --- a/scripts/checkpatch.pl
> > > +++ b/scripts/checkpatch.pl
> > > @@ -3016,7 +3016,8 @@ sub process {
> > >
> > > $herectx .= raw_line($linenr, $n) . "\n";
> > >
> > > }
> > >
> > > - if (($stmts =~ tr/;/;/) == 1) {
> > > + if (($stmts =~ tr/;/;/) == 1 &&
> > > + $stmts !~ /^\s*(if|while|for|switch)\b/) {
> > >
> > > WARN("SINGLE_STATEMENT_DO_WHILE_MACRO",
> > >
> > > "Single statement macros should not use a do {} while
> (0)
> > > loop\n" . "$herectx");>
> > > }
> >
> > Looks reasonable enough.
> >
> > Acked-by: Andy Whitcroft <[email protected]>
>
> Works well for me, thanks
>
> Just as note: I think this test was accidently added twice. Once in commit
> b13edf7ff2dd0fef95e981170fa71fa6b60421b0 and one time in
> 7d64ef43914a734990e48a80bc60686c8db21946 (just checked linux-next, and Stephen
> added it again). Maybe Andrew Morton's tree is out of sync.
This can happen with Andrew's stuff since I rebase it onto linux-next
each day. The rebase is sometimes happy to apply already applied
patches. I will remove the second one today (unless Andrew beats me to
it).
--
Cheers,
Stephen Rothwell [email protected]