Message-ID: <CAFGhKbwBhmM5+aHZrpJLEgpdW4VcND0h3+91QyD69d9b_gEXSw@mail.gmail.com>
Date:   Mon, 20 Feb 2017 18:38:47 +0100
From:   Charlemagne Lasse <charlemagnelasse@...il.com>
To:     Joe Perches <joe@...ches.com>, Andy Whitcroft <apw@...onical.com>
Cc:     linux-kernel@...r.kernel.org
Subject: checkpatch.pl: CHECK: Macro argument 'member' may be better as
 '(member)' to avoid precedence issues

Hi,

I've been playing around with the current checkpatch.pl but I start to
wonder whether the two new checks "CHECK: Macro argument reuse
'member' - possible side-effects?" and "CHECK: Macro argument 'member'
may be better as '(member)' to avoid precedence issues" are correct.

My impression is that they should not apply for all $Operators.At
least "->" and "." seems to be wrong. Here an example using
container_of from include/linux/kernel.h

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
#include <stddef.h>

#define container_of(ptr, type, member) ({                        \
        const typeof(((type *)0)->member) *__mptr = (ptr);        \
        (type *)((char *)__mptr - offsetof(type, member));        \
})

struct list {
        struct list *next;
};

struct foo {
        int b;
        struct list list;
};

struct foo *something(struct list *item)
{
        return container_of(item, struct foo, list);
}
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Checkpatch is generating following warnings:

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
$ ./linux-next/scripts/checkpatch.pl --strict -f test.c
CHECK: Macro argument reuse 'member' - possible side-effects?
#3: FILE: test.c:3:
+#define container_of(ptr, type, member) ({                     \
+       const typeof(((type *)0)->member) *__mptr = (ptr);      \
+       (type *)((char *)__mptr - offsetof(type, member));      \
+})

CHECK: Macro argument 'member' may be better as '(member)' to avoid
precedence issues
#3: FILE: test.c:3:
+#define container_of(ptr, type, member) ({                     \
+       const typeof(((type *)0)->member) *__mptr = (ptr);      \
+       (type *)((char *)__mptr - offsetof(type, member));      \
+})

CHECK: spaces preferred around that '*' (ctx:WxV)
#4: FILE: test.c:4:
+       const typeof(((type *)0)->member) *__mptr = (ptr);      \
                                         ^

total: 0 errors, 0 warnings, 3 checks, 20 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
     mechanically convert to the typical style using --fix or --fix-inplace.

test.c has style problems, please review.

NOTE: If any of the errors are false positives, please report
     them to the maintainer, see CHECKPATCH in MAINTAINERS.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The first one seems to be bogus because it cannot be a variable,
function or statement - but a name of the struct member.

The second one is more interesting. Because -> is allowed as operator
before member, the check will tell the user that it should modify the
macro the following way:

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
#define container_of(ptr, type, member) ({                        \
        const typeof(((type *)0)->(member)) *__mptr = (ptr);        \
        (type *)((char *)__mptr - offsetof(type, (member)));        \
})
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This will result in following error:

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
$ gcc -Wall -W -c test.c -o test.o
test.c: In function ‘something’:
test.c:4:35: error: expected identifier before ‘(’ token
        const typeof(((type *)0)->(member)) *__mptr = (ptr);        \
                                  ^
test.c:19:16: note: in expansion of macro ‘container_of’
        return container_of(item, struct foo, list);
               ^~~~~~~~~~~~
test.c:4:55: warning: initialization from incompatible pointer type
[-Wincompatible-pointer-types]
        const typeof(((type *)0)->(member)) *__mptr = (ptr);        \
                                                      ^
test.c:19:16: note: in expansion of macro ‘container_of’
        return container_of(item, struct foo, list);
               ^~~~~~~~~~~~
In file included from test.c:1:0:
test.c:5:50: error: expected identifier before ‘(’ token
        (type *)((char *)__mptr - offsetof(type, (member)));        \
                                                 ^
test.c:19:16: note: in expansion of macro ‘container_of’
        return container_of(item, struct foo, list);
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

How do I correctly avoid precedence issues for member? Or should the
checks be changed to

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4887,15 +4887,18 @@ sub process {
                               $tmp =~
s/\b(typeof|__typeof__|__builtin\w+|typecheck\s*\(\s*$Type\s*,|\#+)\s*\(*\s*$arg\s*\)*\b//g;
                               $tmp =~ s/\#+\s*$arg\b//g;
                               $tmp =~ s/\b$arg\s*\#\#//g;
+                               my $tmp_member = $define_stmt;
                               my $use_cnt = $tmp =~ s/\b$arg\b//g;
-                               if ($use_cnt > 1) {
+                               my $member_cnt = $tmp_member =~
s/(\.|->)\b$arg\b//g;
+                               if ($use_cnt > 1 && $member_cnt == 0) {
                                       CHK("MACRO_ARG_REUSE",
                                           "Macro argument reuse
'$arg' - possible side-effects?\n" . "$herectx");
                                   }
# check if any macro arguments may have other precedence issues
                               if ($define_stmt =~
m/($Operators)?\s*\b$arg\b\s*($Operators)?/m &&
                                   ((defined($1) && $1 ne ',') ||
-                                    (defined($2) && $2 ne ','))) {
+                                    (defined($2) && $2 ne ',')) &&
+                                    (!defined($1) || ($1 ne '.' && $1
ne '->'))) {
                                       CHK("MACRO_ARG_PRECEDENCE",
                                           "Macro argument '$arg' may
be better as '($arg)' to avoid precedence issues\n" . "$herectx");
                               }

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Powered by blists - more mailing lists