checkpatch.pl: Wrong check SINGLE_STATEMENT_DO_WHILE_MACRO

PATCH In review Schrober 4 participants 6 messages Aug 2, 2012 Checkpatch

lore

Share & tools

lore

Schrober

author

2012-08-02 08:06 UTC #0 lore raw

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.

Signature · 1 line

Franz Schrober

Andy Whitcroft

maintainer

2012-08-02 14:20 UTC #1 lore raw

Quoted 24 lines · Schrober

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.

Quoted 6 lines

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

Joe Perches

maintainer

2012-08-02 15:26 UTC #2 lore raw

PATCH 1/1 checkpatch: Add control statement test to SINGLE_STATEMENT_DO_WHILE_MACRO

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

---

Quoted 2 lines · Andy Whitcroft

Quoted 1 line · Schrober
Hi,

Hello and thanks for the report.

Quoted 1 line

Quoted 1 line
I think your check for SINGLE_STATEMENT_DO_WHILE_MACRO is wrong.

And you are correct.

Quoted 2 lines

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(-)

scripts/checkpatch.pl +2 -1

index 913d6bd..ca05ba2 100755
@@ -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");
 				}

Andy Whitcroft

maintainer Acked-by on v1 1/1

2012-08-02 16:23 UTC #3 lore raw

Re: [PATCH] checkpatch: Add control statement test to SINGLE_STATEMENT_DO_WHILE_MACRO

Quoted 51 lines · Joe Perches

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(-)

scripts/checkpatch.pl +0 -0

index 913d6bd..ca05ba2 100755

scripts/checkpatch.pl sub process { · line 3023

Quoted 9 lines

scripts/checkpatch.pl +2 -1

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

Schrober

author

2012-08-02 16:45 UTC #4 lore raw

Re: [PATCH] checkpatch: Add control statement test to SINGLE_STATEMENT_DO_WHILE_MACRO

Quoted 10 lines · On Thursday 02 August 2012 17:23:22 Andy Whitcroft wrote:

Quoted 10 lines

scripts/checkpatch.pl +0 -0

index 913d6bd..ca05ba2 100755
@@ -3016,7 +3016,8 @@ sub process {
 
 					$herectx .= raw_line($linenr, $n) . "\n";
 
 				}

Quoted 7 lines

scripts/checkpatch.pl

Quoted 7 lines

scripts/checkpatch.pl +2 -1

-				if (($stmts =~ tr/;/;/) == 1) {
+				if (($stmts =~ tr/;/;/) == 1 &&
+				    $stmts !~ /^\s*(if|while|for|switch)\b/) {

scripts/checkpatch.pl +0 -0

 					WARN("SINGLE_STATEMENT_DO_WHILE_MACRO",
 
 					     "Single statement macros should not use a do {} while

(0)

Quoted 6 lines

scripts/checkpatch.pl
Quoted 2 lines
scripts/checkpatch.pl +0 -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.

Signature · 1 line

Franz Schrober

Stephen Rothwell

2012-08-02 22:10 UTC #5 lore raw

Re: [PATCH] checkpatch: Add control statement test to SINGLE_STATEMENT_DO_WHILE_MACRO

Hi Franz,

Quoted 25 lines · On Thu, 02 Aug 2012 18:44:57 +0200 Schrober <[email protected]> wrote:

Quoted 10 lines · On Thursday 02 August 2012 17:23:22 Andy Whitcroft wrote:
Quoted 10 lines
scripts/checkpatch.pl +0 -0
index 913d6bd..ca05ba2 100755
@@ -3016,7 +3016,8 @@ sub process {
 
 					$herectx .= raw_line($linenr, $n) . "\n";
 
 				}
 
Quoted 7 lines
scripts/checkpatch.pl
Quoted 7 lines
scripts/checkpatch.pl +2 -1
-				if (($stmts =~ tr/;/;/) == 1) {
+				if (($stmts =~ tr/;/;/) == 1 &&
+				    $stmts !~ /^\s*(if|while|for|switch)\b/) {
 
scripts/checkpatch.pl +0 -0
 					WARN("SINGLE_STATEMENT_DO_WHILE_MACRO",
 
 					     "Single statement macros should not use a do {} while 
(0)
Quoted 5 lines
scripts/checkpatch.pl
Quoted 2 lines
scripts/checkpatch.pl +0 -0
 					     loop\n" . "$herectx");>  				
 				}
Looks reasonable enough.

Quoted 8 lines

Quoted 1 line
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).

Signature · 2 lines

Cheers,
Stephen Rothwell                    [email protected]