Possible false positive from checkpatch.pl

Possible false positive from checkpatch.pl

[Larry Finger]

Andy,

In preparing a vendor driver for submission to staging, I am getting the 
following from checkpatch.pl:

ERROR: Macros with multiple statements should be enclosed in a do - while loop
#377: FILE: staging/rtl8712/rtl871x_mp_ioctl.h:377:
+#define GEN_MP_IOCTL_HANDLER(sz, hdl, oid) {sz, hdl, oid},

ERROR: Macros with multiple statements should be enclosed in a do - while loop
#378: FILE: staging/rtl8712/rtl871x_mp_ioctl.h:378:
+#define EXT_MP_IOCTL_HANDLER(sz, subcode, oid) {sz, &mp_ioctl_ \
+       ## subcode ## _hdl, oid},

total: 2 errors, 0 warnings, 466 lines checked

Enclosing these macros in a do {...} while (0) is definitely wrong and will not 
compile. Moving the comma from the end of the macro to the lines that invoke it 
fixes the problem, but should not be necessary.

Thanks,

Re: Possible false positive from checkpatch.pl

[Joe Perches]

On Mon, 2010-07-12 at 12:52 -0500, Larry Finger wrote:
> Andy,
> 
> In preparing a vendor driver for submission to staging, I am getting the 
> following from checkpatch.pl:
> 
> ERROR: Macros with multiple statements should be enclosed in a do - while loop
> #377: FILE: staging/rtl8712/rtl871x_mp_ioctl.h:377:
> +#define GEN_MP_IOCTL_HANDLER(sz, hdl, oid) {sz, hdl, oid},

I think you should leave off the trailing comma from the macros
and C99 might be better.  Maybe something like:

(whatever the field names really are)

#define GEN_MP_IOCTL_HANDLER(sz, hdl, oid)	\
	{.field1 = sz, .field2 = hdl, .field3 = oid}

> ERROR: Macros with multiple statements should be enclosed in a do - while loop
> #378: FILE: staging/rtl8712/rtl871x_mp_ioctl.h:378:
> +#define EXT_MP_IOCTL_HANDLER(sz, subcode, oid) {sz, &mp_ioctl_ \
> +       ## subcode ## _hdl, oid},

The line continuation is rather ugly too.  Perhaps it's better as:

#define EXT_MP_IOCTL_HANDLER(sz, subcode, oid)	\
	{.field1 = sz, .field2 = &mp_ioctl_##subcode##_hdl, .field3 = oid}

They pass checkpatch without error.

$ cat foo.h
#define GEN_MP_IOCTL_HANDLER(sz, hdl, oid)	\
	{.field1 = sz, .field2 = hdl, .field3 = oid}
#define EXT_MP_IOCTL_HANDLER(sz, subcode, oid)	\
	{.field1 = sz, .field2 = &mp_ioctl_##subcode##_hdl, .field3 = oid}
$ ./scripts/checkpatch.pl -f foo.h
total: 0 errors, 0 warnings, 4 lines checked

foo.h has no obvious style problems and is ready for submission.

Re: Possible false positive from checkpatch.pl

[Larry Finger]

On 07/12/2010 01:28 PM, Joe Perches wrote:
> On Mon, 2010-07-12 at 12:52 -0500, Larry Finger wrote:
>> Andy,
>>
>> In preparing a vendor driver for submission to staging, I am getting the
>> following from checkpatch.pl:
>>
>> ERROR: Macros with multiple statements should be enclosed in a do - while loop
>> #377: FILE: staging/rtl8712/rtl871x_mp_ioctl.h:377:
>> +#define GEN_MP_IOCTL_HANDLER(sz, hdl, oid) {sz, hdl, oid},
>
> I think you should leave off the trailing comma from the macros
> and C99 might be better.  Maybe something like:
>
> (whatever the field names really are)
>
> #define GEN_MP_IOCTL_HANDLER(sz, hdl, oid)	\
> 	{.field1 = sz, .field2 = hdl, .field3 = oid}
>
>> ERROR: Macros with multiple statements should be enclosed in a do - while loop
>> #378: FILE: staging/rtl8712/rtl871x_mp_ioctl.h:378:
>> +#define EXT_MP_IOCTL_HANDLER(sz, subcode, oid) {sz,&mp_ioctl_ \
>> +       ## subcode ## _hdl, oid},
>
> The line continuation is rather ugly too.  Perhaps it's better as:
>
> #define EXT_MP_IOCTL_HANDLER(sz, subcode, oid)	\
> 	{.field1 = sz, .field2 =&mp_ioctl_##subcode##_hdl, .field3 = oid}
>
> They pass checkpatch without error.
>
> $ cat foo.h
> #define GEN_MP_IOCTL_HANDLER(sz, hdl, oid)	\
> 	{.field1 = sz, .field2 = hdl, .field3 = oid}
> #define EXT_MP_IOCTL_HANDLER(sz, subcode, oid)	\
> 	{.field1 = sz, .field2 =&mp_ioctl_##subcode##_hdl, .field3 = oid}
> $ ./scripts/checkpatch.pl -f foo.h
> total: 0 errors, 0 warnings, 4 lines checked
>
> foo.h has no obvious style problems and is ready for submission.

These are ugly macros that will be eliminated, but for the moment they are in 
the code. As I stated in my original email, removing the comma from the 
definition and adding it to the code does fix the checkpatch error, but it 
should not be necessary.

Larry

Re: Possible false positive from checkpatch.pl

[Joe Perches]

On Mon, 2010-07-12 at 13:35 -0500, Larry Finger wrote:
> These are ugly macros that will be eliminated, but for the moment they are in 
> the code. As I stated in my original email, removing the comma from the 
> definition and adding it to the code does fix the checkpatch error, but it 
> should not be necessary.

Hi Larry.

Using checkpatch is not necessary.

If you want generally conforming kernel style,
the macro should not end in a trailing comma.

Feel free to ignore the checkpatch message,

I think the warning is reasonable, though it
could be made more specific.

cheers, Joe

Maybe something like:
---
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index bd88f11..7e8a3f4 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2394,8 +2394,10 @@ sub process {
 			}x;
 			#print "REST<$rest> dstat<$dstat>\n";
 			if ($rest ne '') {
-				if ($rest !~ /while\s*\(/ &&
-				    $dstat !~ /$exceptions/)
+				if ($rest eq ",") {
+					ERROR("Macros should not end with a trailing comma\n" . "$here\n$ctx\n");
+				} elsif ($rest !~ /while\s*\(/ &&
+					 $dstat !~ /$exceptions/)
 				{
 					ERROR("Macros with multiple statements should be enclosed in a do - while loop\n" . "$here\n$ctx\n");
 				}

Re: Possible false positive from checkpatch.pl

[Larry Finger]

On 07/12/2010 02:03 PM, Joe Perches wrote:
> On Mon, 2010-07-12 at 13:35 -0500, Larry Finger wrote:
>> These are ugly macros that will be eliminated, but for the moment they are in
>> the code. As I stated in my original email, removing the comma from the
>> definition and adding it to the code does fix the checkpatch error, but it
>> should not be necessary.
>
> Hi Larry.
>
> Using checkpatch is not necessary.
>
> If you want generally conforming kernel style,
> the macro should not end in a trailing comma.
>
> Feel free to ignore the checkpatch message,
>
> I think the warning is reasonable, though it
> could be made more specific.
>
> cheers, Joe
>
> Maybe something like:
> ---
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index bd88f11..7e8a3f4 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2394,8 +2394,10 @@ sub process {
>   			}x;
>   			#print "REST<$rest>  dstat<$dstat>\n";
>   			if ($rest ne '') {
> -				if ($rest !~ /while\s*\(/&&
> -				    $dstat !~ /$exceptions/)
> +				if ($rest eq ",") {
> +					ERROR("Macros should not end with a trailing comma\n" . "$here\n$ctx\n");
> +				} elsif ($rest !~ /while\s*\(/&&
> +					 $dstat !~ /$exceptions/)
>   				{
>   					ERROR("Macros with multiple statements should be enclosed in a do - while loop\n" . "$here\n$ctx\n");
>   			}

That looks good. At least it makes clear what is wrong.

Should it be an error, or just a warning?

Larry

Re: Possible false positive from checkpatch.pl

[Joe Perches]

On Mon, 2010-07-12 at 14:37 -0500, Larry Finger wrote:
> On 07/12/2010 02:03 PM, Joe Perches wrote:
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index bd88f11..7e8a3f4 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -2394,8 +2394,10 @@ sub process {
> >   			}x;
> >   			#print "REST<$rest>  dstat<$dstat>\n";
> >   			if ($rest ne '') {
> > -				if ($rest !~ /while\s*\(/&&
> > -				    $dstat !~ /$exceptions/)
> > +				if ($rest eq ",") {
> > +					ERROR("Macros should not end with a trailing comma\n" . "$here\n$ctx\n");
> > +				} elsif ($rest !~ /while\s*\(/&&
> > +					 $dstat !~ /$exceptions/)
> >   				{
> >   					ERROR("Macros with multiple statements should be enclosed in a do - while loop\n" . "$here\n$ctx\n");
> >   			}
> 
> That looks good. At least it makes clear what is wrong.
> Should it be an error, or just a warning?

I don't much care.

If Andy wants to do anything, let him decide.

Perhaps the new test should be
	if ($rest =~ /\s*,\s*$/)

cheers, Joe

Re: Possible false positive from checkpatch.pl

[Andy Whitcroft]

On Mon, Jul 12, 2010 at 12:49:07PM -0700, Joe Perches wrote:
> On Mon, 2010-07-12 at 14:37 -0500, Larry Finger wrote:
> > On 07/12/2010 02:03 PM, Joe Perches wrote:
> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > > index bd88f11..7e8a3f4 100755
> > > --- a/scripts/checkpatch.pl
> > > +++ b/scripts/checkpatch.pl
> > > @@ -2394,8 +2394,10 @@ sub process {
> > >   			}x;
> > >   			#print "REST<$rest>  dstat<$dstat>\n";
> > >   			if ($rest ne '') {
> > > -				if ($rest !~ /while\s*\(/&&
> > > -				    $dstat !~ /$exceptions/)
> > > +				if ($rest eq ",") {
> > > +					ERROR("Macros should not end with a trailing comma\n" . "$here\n$ctx\n");
> > > +				} elsif ($rest !~ /while\s*\(/&&
> > > +					 $dstat !~ /$exceptions/)
> > >   				{
> > >   					ERROR("Macros with multiple statements should be enclosed in a do - while loop\n" . "$here\n$ctx\n");
> > >   			}
> > 
> > That looks good. At least it makes clear what is wrong.
> > Should it be an error, or just a warning?
> 
> I don't much care.
> 
> If Andy wants to do anything, let him decide.
> 
> Perhaps the new test should be
> 	if ($rest =~ /\s*,\s*$/)

I am not sure these are even invalid are they?  Macro abuse of this sort
is always going to throw up exceptions.  Hrmm.

-apw

Re: Possible false positive from checkpatch.pl

[Joe Perches]

On Wed, 2010-07-14 at 14:14 +0100, Andy Whitcroft wrote:
> On Mon, Jul 12, 2010 at 12:49:07PM -0700, Joe Perches wrote:
> > On Mon, 2010-07-12 at 14:37 -0500, Larry Finger wrote:
> > > That looks good. At least it makes clear what is wrong.
> > > Should it be an error, or just a warning?
> > I don't much care.
> > If Andy wants to do anything, let him decide.
> > Perhaps the new test should be
> > 	if ($rest =~ /\s*,\s*$/)
> I am not sure these are even invalid are they?  Macro abuse of this sort
> is always going to throw up exceptions.  Hrmm.

This seems to work better.

---
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index bd88f11..548f8d9 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2393,6 +2393,9 @@ sub process {
 				^\"|\"$
 			}x;
 			#print "REST<$rest> dstat<$dstat>\n";
+			if ($rest =~ /,\s*$/ || $dstat =~ /,\s*$/) {
+			    ERROR("Macros should not end with a trailing comma\n" . "$here\n$ctx\n");
+			}
 			if ($rest ne '') {
 				if ($rest !~ /while\s*\(/ &&
 				    $dstat !~ /$exceptions/)

Re: Possible false positive from checkpatch.pl

[Andy Whitcroft]

On Mon, Jul 12, 2010 at 12:52:32PM -0500, Larry Finger wrote:
> Andy,
> 
> In preparing a vendor driver for submission to staging, I am getting
> the following from checkpatch.pl:
> 
> ERROR: Macros with multiple statements should be enclosed in a do - while loop
> #377: FILE: staging/rtl8712/rtl871x_mp_ioctl.h:377:
> +#define GEN_MP_IOCTL_HANDLER(sz, hdl, oid) {sz, hdl, oid},
> 
> ERROR: Macros with multiple statements should be enclosed in a do - while loop
> #378: FILE: staging/rtl8712/rtl871x_mp_ioctl.h:378:
> +#define EXT_MP_IOCTL_HANDLER(sz, subcode, oid) {sz, &mp_ioctl_ \
> +       ## subcode ## _hdl, oid},
> 
> total: 2 errors, 0 warnings, 466 lines checked
> 
> Enclosing these macros in a do {...} while (0) is definitely wrong
> and will not compile. Moving the comma from the end of the macro to
> the lines that invoke it fixes the problem, but should not be
> necessary.

They do look like a false positive to me.  Its not entirly obvious id we
cna detect them as valid exceptions.  Always remember that checkpatch is
an advisor, if its wrong it can and should be ignored.

I will think on this some.

-apw