| Message ID | 20171130140154.11161-1-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | macro do/while (0) cleanup | expand |
On 11/30/2017 08:01 AM, Eric Blake wrote: > while (0) is only idiomatic in a macro definition, where the caller > will be supplying the trailing ';'. Warn if the macro has a duplicate. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > scripts/checkpatch.pl | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index 34df753571..acb66bff34 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -1622,6 +1622,11 @@ sub process { > } > } > > +# 'while (0);' is odd; only macros should use while (0), without trailing ; > + if ($line =~ /while\s*\(0\);/) { Should this also check for uses of 'while (false);' ? Interestingly enough, we have an instance of 'do/while (false);' in tests/vhost-user-bridge.c that is NOT in a macro, but is used for the convenience of being able to 'break;' out early rather than using a goto. Similarly for chardev/char-serial.c using 'while (0);' outside of a macro. Those may be worth rewriting to use goto as separate patches if we want to restrict ALL use of 'while \((0|false)\);'
Eric Blake <eblake@redhat.com> writes: > On 11/30/2017 08:01 AM, Eric Blake wrote: >> while (0) is only idiomatic in a macro definition, where the caller >> will be supplying the trailing ';'. Warn if the macro has a duplicate. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> --- >> scripts/checkpatch.pl | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl >> index 34df753571..acb66bff34 100755 >> --- a/scripts/checkpatch.pl >> +++ b/scripts/checkpatch.pl >> @@ -1622,6 +1622,11 @@ sub process { >> } >> } >> >> +# 'while (0);' is odd; only macros should use while (0), without trailing ; >> + if ($line =~ /while\s*\(0\);/) { > > Should this also check for uses of 'while (false);' ? Do we think it's likely to occur? > Interestingly enough, we have an instance of 'do/while (false);' in > tests/vhost-user-bridge.c that is NOT in a macro, but is used for the > convenience of being able to 'break;' out early rather than using a > goto. Similarly for chardev/char-serial.c using 'while (0);' outside > of a macro. That "cure" merely adds gratuitous cleverness to the "disease". > Those may be worth rewriting to use goto as separate > patches if we want to restrict ALL use of 'while \((0|false)\);' I'd support that.
On 12/01/2017 01:31 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> On 11/30/2017 08:01 AM, Eric Blake wrote: >>> while (0) is only idiomatic in a macro definition, where the caller >>> will be supplying the trailing ';'. Warn if the macro has a duplicate. >>> >>> >>> +# 'while (0);' is odd; only macros should use while (0), without trailing ; >>> + if ($line =~ /while\s*\(0\);/) { >> >> Should this also check for uses of 'while (false);' ? > > Do we think it's likely to occur? Not as frequent, but it does appear in the code base. > >> Interestingly enough, we have an instance of 'do/while (false);' in >> tests/vhost-user-bridge.c that is NOT in a macro, but is used for the >> convenience of being able to 'break;' out early rather than using a >> goto. Similarly for chardev/char-serial.c using 'while (0);' outside >> of a macro. > > That "cure" merely adds gratuitous cleverness to the "disease". > >> Those may be worth rewriting to use goto as separate >> patches if we want to restrict ALL use of 'while \((0|false)\);' > > I'd support that. > Okay, I'll post a v2 along those lines.
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 34df753571..acb66bff34 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -1622,6 +1622,11 @@ sub process { } } +# 'while (0);' is odd; only macros should use while (0), without trailing ; + if ($line =~ /while\s*\(0\);/) { + ERROR("suspicious ; after while (0)\n" . $herecurr); + } + # Check relative indent for conditionals and blocks. if ($line =~ /\b(?:(?:if|while|for)\s*\(|do\b)/ && $line !~ /^.\s*#/ && $line !~ /\}\s*while\s*/) { my ($s, $c) = ($stat, $cond);
while (0) is only idiomatic in a macro definition, where the caller will be supplying the trailing ';'. Warn if the macro has a duplicate. Signed-off-by: Eric Blake <eblake@redhat.com> --- scripts/checkpatch.pl | 5 +++++ 1 file changed, 5 insertions(+)