* Possible false positive from checkpatch.pl
@ 2010-07-12 17:52 Larry Finger
2010-07-12 18:28 ` Joe Perches
2010-07-14 13:13 ` Andy Whitcroft
0 siblings, 2 replies; 9+ messages in thread
From: Larry Finger @ 2010-07-12 17:52 UTC (permalink / raw)
To: Andy Whitcroft; +Cc: LKML
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,
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: Possible false positive from checkpatch.pl 2010-07-12 17:52 Possible false positive from checkpatch.pl Larry Finger @ 2010-07-12 18:28 ` Joe Perches 2010-07-12 18:35 ` Larry Finger 2010-07-14 13:13 ` Andy Whitcroft 1 sibling, 1 reply; 9+ messages in thread From: Joe Perches @ 2010-07-12 18:28 UTC (permalink / raw) To: Larry Finger; +Cc: Andy Whitcroft, LKML 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Possible false positive from checkpatch.pl 2010-07-12 18:28 ` Joe Perches @ 2010-07-12 18:35 ` Larry Finger 2010-07-12 19:03 ` Joe Perches 0 siblings, 1 reply; 9+ messages in thread From: Larry Finger @ 2010-07-12 18:35 UTC (permalink / raw) To: Joe Perches; +Cc: Andy Whitcroft, LKML 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Possible false positive from checkpatch.pl 2010-07-12 18:35 ` Larry Finger @ 2010-07-12 19:03 ` Joe Perches 2010-07-12 19:37 ` Larry Finger 0 siblings, 1 reply; 9+ messages in thread From: Joe Perches @ 2010-07-12 19:03 UTC (permalink / raw) To: Larry Finger; +Cc: Andy Whitcroft, LKML 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"); } ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: Possible false positive from checkpatch.pl 2010-07-12 19:03 ` Joe Perches @ 2010-07-12 19:37 ` Larry Finger 2010-07-12 19:49 ` Joe Perches 0 siblings, 1 reply; 9+ messages in thread From: Larry Finger @ 2010-07-12 19:37 UTC (permalink / raw) To: Joe Perches; +Cc: Andy Whitcroft, LKML 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Possible false positive from checkpatch.pl 2010-07-12 19:37 ` Larry Finger @ 2010-07-12 19:49 ` Joe Perches 2010-07-14 13:14 ` Andy Whitcroft 0 siblings, 1 reply; 9+ messages in thread From: Joe Perches @ 2010-07-12 19:49 UTC (permalink / raw) To: Larry Finger; +Cc: Andy Whitcroft, LKML 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Possible false positive from checkpatch.pl 2010-07-12 19:49 ` Joe Perches @ 2010-07-14 13:14 ` Andy Whitcroft 2010-07-14 16:16 ` Joe Perches 0 siblings, 1 reply; 9+ messages in thread From: Andy Whitcroft @ 2010-07-14 13:14 UTC (permalink / raw) To: Joe Perches; +Cc: Larry Finger, LKML 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Possible false positive from checkpatch.pl 2010-07-14 13:14 ` Andy Whitcroft @ 2010-07-14 16:16 ` Joe Perches 0 siblings, 0 replies; 9+ messages in thread From: Joe Perches @ 2010-07-14 16:16 UTC (permalink / raw) To: Andy Whitcroft; +Cc: Larry Finger, LKML 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/) ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: Possible false positive from checkpatch.pl 2010-07-12 17:52 Possible false positive from checkpatch.pl Larry Finger 2010-07-12 18:28 ` Joe Perches @ 2010-07-14 13:13 ` Andy Whitcroft 1 sibling, 0 replies; 9+ messages in thread From: Andy Whitcroft @ 2010-07-14 13:13 UTC (permalink / raw) To: Larry Finger; +Cc: LKML 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2010-07-14 16:16 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-07-12 17:52 Possible false positive from checkpatch.pl Larry Finger 2010-07-12 18:28 ` Joe Perches 2010-07-12 18:35 ` Larry Finger 2010-07-12 19:03 ` Joe Perches 2010-07-12 19:37 ` Larry Finger 2010-07-12 19:49 ` Joe Perches 2010-07-14 13:14 ` Andy Whitcroft 2010-07-14 16:16 ` Joe Perches 2010-07-14 13:13 ` Andy Whitcroft
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.