Prev: ACPI: processor: fix processor_physically_present on UP
Next: [PATCH] sound/oss: convert to unlocked_ioctl
From: Larry Finger on 12 Jul 2010 14:00
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,

--
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 12 Jul 2010 14:30
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.

--
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: Larry Finger on 12 Jul 2010 14:40
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
--
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 12 Jul 2010 15:10
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");
}



--
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: Larry Finger on 12 Jul 2010 15:40
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
--
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/
 |  Next  |  Last
Pages: 1 2
Prev: ACPI: processor: fix processor_physically_present on UP
Next: [PATCH] sound/oss: convert to unlocked_ioctl