Messages in this thread Patch in this message | | | From | Charlemagne Lasse <> | Date | Mon, 20 Feb 2017 18:38:47 +0100 | 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"); }
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| |