Prev: ACPI: processor: fix processor_physically_present on UP
Next: [PATCH] sound/oss: convert to unlocked_ioctl
From: Joe Perches on 12 Jul 2010 15:50
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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Andy Whitcroft on 14 Jul 2010 09:20
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Joe Perches on 14 Jul 2010 12:20
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/)



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
First  |  Prev  | 
Pages: 1 2
Prev: ACPI: processor: fix processor_physically_present on UP
Next: [PATCH] sound/oss: convert to unlocked_ioctl