* checkpatch.pl: Wrong check SINGLE_STATEMENT_DO_WHILE_MACRO
@ 2012-08-02 8:00 Schrober
2012-08-02 14:20 ` Andy Whitcroft
0 siblings, 1 reply; 6+ messages in thread
From: Schrober @ 2012-08-02 8:00 UTC (permalink / raw)
To: Joe Perches; +Cc: Andy Whitcroft, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: checkpatch.pl: Wrong check SINGLE_STATEMENT_DO_WHILE_MACRO
2012-08-02 8:00 checkpatch.pl: Wrong check SINGLE_STATEMENT_DO_WHILE_MACRO Schrober
@ 2012-08-02 14:20 ` Andy Whitcroft
2012-08-02 15:26 ` [PATCH] checkpatch: Add control statement test to SINGLE_STATEMENT_DO_WHILE_MACRO Joe Perches
0 siblings, 1 reply; 6+ messages in thread
From: Andy Whitcroft @ 2012-08-02 14:20 UTC (permalink / raw)
To: Schrober; +Cc: Joe Perches, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] checkpatch: Add control statement test to SINGLE_STATEMENT_DO_WHILE_MACRO
2012-08-02 14:20 ` Andy Whitcroft
@ 2012-08-02 15:26 ` Joe Perches
2012-08-02 16:23 ` Andy Whitcroft
0 siblings, 1 reply; 6+ messages in thread
From: Joe Perches @ 2012-08-02 15:26 UTC (permalink / raw)
To: Schrober, Andy Whitcroft, Andrew Morton; +Cc: linux-kernel
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 <joe@perches.com>
---
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");
}
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] checkpatch: Add control statement test to SINGLE_STATEMENT_DO_WHILE_MACRO
2012-08-02 15:26 ` [PATCH] checkpatch: Add control statement test to SINGLE_STATEMENT_DO_WHILE_MACRO Joe Perches
@ 2012-08-02 16:23 ` Andy Whitcroft
2012-08-02 16:44 ` Schrober
0 siblings, 1 reply; 6+ messages in thread
From: Andy Whitcroft @ 2012-08-02 16:23 UTC (permalink / raw)
To: Joe Perches; +Cc: Schrober, Andrew Morton, linux-kernel
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 <apw@canonical.com>
-apw
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] checkpatch: Add control statement test to SINGLE_STATEMENT_DO_WHILE_MACRO
2012-08-02 16:23 ` Andy Whitcroft
@ 2012-08-02 16:44 ` Schrober
2012-08-02 22:10 ` Stephen Rothwell
0 siblings, 1 reply; 6+ messages in thread
From: Schrober @ 2012-08-02 16:44 UTC (permalink / raw)
To: Andy Whitcroft
Cc: Joe Perches, Andrew Morton, linux-kernel, Stephen Rothwell,
Andrew Morton
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 <apw@canonical.com>
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] checkpatch: Add control statement test to SINGLE_STATEMENT_DO_WHILE_MACRO
2012-08-02 16:44 ` Schrober
@ 2012-08-02 22:10 ` Stephen Rothwell
0 siblings, 0 replies; 6+ messages in thread
From: Stephen Rothwell @ 2012-08-02 22:10 UTC (permalink / raw)
To: Schrober; +Cc: Andy Whitcroft, Joe Perches, Andrew Morton, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1540 bytes --]
Hi Franz,
On Thu, 02 Aug 2012 18:44:57 +0200 Schrober <franzschrober@yahoo.de> 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 <apw@canonical.com>
>
> 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 sfr@canb.auug.org.au
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-08-02 22:10 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-08-02 8:00 checkpatch.pl: Wrong check SINGLE_STATEMENT_DO_WHILE_MACRO Schrober
2012-08-02 14:20 ` Andy Whitcroft
2012-08-02 15:26 ` [PATCH] checkpatch: Add control statement test to SINGLE_STATEMENT_DO_WHILE_MACRO Joe Perches
2012-08-02 16:23 ` Andy Whitcroft
2012-08-02 16:44 ` Schrober
2012-08-02 22:10 ` Stephen Rothwell
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).