Re: [Qemu-arm] checkpatch.pl false positive (was Re: [PATCH v4 0/7] q35: add negotiable broadcast SMI)

[Eduardo Habkost]

On Wed, Dec 21, 2016 at 06:47:01PM +0100, Paolo Bonzini wrote:
> 
> 
> On 21/12/2016 16:22, Eduardo Habkost wrote:
> > On Tue, Dec 20, 2016 at 03:01:17PM -0800, address@hidden wrote:
> > [...]
> >> Checking PATCH 4/7: hw/i386/pc: introduce 2.9 machine types with 0x20 
> >> fw_cfg file slots...
> >> ERROR: Macros with multiple statements should be enclosed in a do - while 
> >> loop
> >> #126: FILE: include/hw/compat.h:4:
> >> +#define HW_COMPAT_2_8 \
> >> +    {\
> >> +        .driver   = "fw_cfg_mem",\
> >> +        .property = "file_slots",\
> >> +        .value    = stringify(0x10),\
> >> +    },{\
> >> +        .driver   = "fw_cfg_io",\
> >> +        .property = "file_slots",\
> >> +        .value    = stringify(0x10),\
> >> +    },
> >>
> >> total: 1 errors, 0 warnings, 119 lines checked
> >>
> >> Your patch has style problems, please review.  If any of these errors
> >> are false positives report them to the maintainer, see
> >> CHECKPATCH in MAINTAINERS.
> > 
> > It is a false positive, but how exactly can we fix it? Should it
> > become a warning instead of an error?
> 
> It should already be treated as an exception:
> 
>                         my $exceptions = qr{
>                                 $Declare|
>                                 module_param_named|
>                                 MODULE_PARAM_DESC|
>                                 DECLARE_PER_CPU|
>                                 DEFINE_PER_CPU|
>                                 __typeof__\(|
>                                 union|
>                                 struct|
>                                 \.$Ident\s*=\s*|       # <---- see here
>                                 ^\"|\"$
>                         }x;
>                         #print "REST<$rest> dstat<$dstat> ctx<$ctx>\n";
>                         if ($rest ne '' && $rest ne ',') {
>                                 if ($rest !~ /while\s*\(/ &&
>                                     $dstat !~ /$exceptions/)
>                                 {
>                                         ERROR("Macros with multiple 
> statements should be enclosed in a do - while loop\n" . "$here\n$ctx\n");
>                                 }
> 
> 
> so I guess the first step is debugging it. :)

The following code replaces the whole "{ .driver = ... }" block
with "1":

        # Flatten any parentheses and braces
        while ($dstat =~ s/\([^\(\)]*\)/1/ ||
               $dstat =~ s/\{[^\{\}]*\}/1/ ||
               $dstat =~ s/\[[^\{\}]*\]/1/)
        {
        }

The following change fixes the bug, but I don't know if it has
unwanted side-effects:

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index f084542..0aab3ac 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2200,6 +2200,10 @@ sub process {
                        $dstat =~ s/^\s*//s;
                        $dstat =~ s/\s*$//s;
 
+                       # remove braces that cover the whole block, if any:
+                       $dstat =~ s/^\{//;
+                       $dstat =~ s/\}$//;
+
                        # Flatten any parentheses and braces
                        while ($dstat =~ s/\([^\(\)]*\)/1/ ||
                               $dstat =~ s/\{[^\{\}]*\}/1/ ||


-- 
Eduardo